Page MenuHomeFreeBSD
Differential D6848

Fix bitstring allocation on 32-bit platforms
ClosedPublic
Actions

Authored by asomers on Jun 14 2016, 11:24 PM.
Tags
None
Referenced Files
F113889050: D6848.id17861.diff
Sat, Apr 5, 3:46 AM
Unknown Object (File)
Sat, Mar 8, 10:10 PM
Unknown Object (File)
Feb 26 2025, 1:15 AM
Unknown Object (File)
Feb 21 2025, 8:16 AM
Unknown Object (File)
Feb 18 2025, 9:49 PM
Unknown Object (File)
Feb 18 2025, 12:31 PM
Unknown Object (File)
Feb 16 2025, 3:51 AM
Unknown Object (File)
Feb 11 2025, 10:35 PM
View All Files
Subscribers
imp

Details

Reviewers
gibbs
Commits
rS302180: Fix bitstring allocation on 32-bit platforms
Summary

sys/sys/bitstring.h
Fix a rounding calculation that could undersize a bitstring on
32-bit platforms.

include/bitstring.h
Add sys/param.h for the roundup2 macro

Test Plan

Existing ATF test cases

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4336
Build 4381: arc lint + arc unit

Event Timeline

asomers updated this revision to Diff 17591.Jun 14 2016, 11:24 PM
asomers retitled this revision from to Fix bitstring allocation on 32-bit platforms.
asomers updated this object.
asomers edited the test plan for this revision. (Show Details)
asomers added a reviewer: gibbs.
Herald added a subscriber: imp. · View Herald TranscriptJun 14 2016, 11:24 PM
gibbs accepted this revision.Jun 14 2016, 11:35 PM
gibbs edited edge metadata.

I see how this can over allocate on platforms with long > 8bits, but I'm missing how 32bit long platforms are special and can get too little space allocated. Can you provide an example in the checkin comment?

This revision is now accepted and ready to land.Jun 14 2016, 11:35 PM
asomers updated this revision to Diff 17600.Jun 15 2016, 5:23 PM
asomers edited edge metadata.

Restore bitstr_size to its originally intended purpose of calculating the
actual number of bytes used by a bitstring, and fix bit_decl to round up
appropriately.

This revision now requires review to proceed.Jun 15 2016, 5:23 PM
asomers updated this revision to Diff 17771.Jun 22 2016, 7:46 PM
asomers edited edge metadata.

The important thing about division is that you divide.

gibbs added a comment.Jun 23 2016, 3:48 PM

If I want to dynamically allocate a bitstr, I use bitstr_size() to tell me how much to allocate. Without the roundup, I'll potentially allocate too little memory.

sys/sys/bitstring.h
114–115

#define bitstr_size(_nbits) (roundup2(_nbits, _BITSTR_BITS) / 8)

134

No roundup is required here. Is it clearer to use howmany()? I'm not sure.

gibbs added a comment.Jun 23 2016, 3:50 PM

Should we inline roundup2? Prior to this change, at least in user space, bitstring.h only relied on functionality provided in the C standard. It might be nice to keep that.

asomers added a comment.Jun 23 2016, 5:14 PM
In D6848#145456, @gibbs wrote:

If I want to dynamically allocate a bitstr, I use bitstr_size() to tell me how much to allocate. Without the roundup, I'll potentially allocate too little memory.

I would say that you should use bit_alloc. But plenty of callers are doing it themselves. I'll do as you suggest, which will basically revert the 2nd diff of this review.

sys/sys/bitstring.h
134

Roundup most certainly is required here, unless the roundup is implemented in bitstr_size instead. howmany() is basically just division rounding up.

gibbs added inline comments.Jun 23 2016, 5:16 PM
sys/sys/bitstring.h
134

That's what I meant. If bitstr_size() has to roundup anyway, there's no need to roundup here too.

asomers updated this revision to Diff 17824.Jun 23 2016, 5:43 PM

Move the roundup back into bitstr_size instead of bit_decl

asomers added a comment.Jun 24 2016, 2:54 PM

Are you happy with this @gibbs? Due to my vacation, I must commit today or it won't get into 11.0.

gibbs accepted this revision.Jun 24 2016, 2:55 PM
gibbs edited edge metadata.

I'm just not sure about the new dependency on sys/param.h. I leave that for you to decide.

This revision is now accepted and ready to land.Jun 24 2016, 2:55 PM
asomers updated this revision to Diff 17861.Jun 24 2016, 4:03 PM
asomers edited edge metadata.

Remove sys/param.h include.

This revision now requires review to proceed.Jun 24 2016, 4:03 PM
gibbs requested changes to this revision.Jun 24 2016, 4:48 PM
gibbs edited edge metadata.
gibbs added inline comments.
sys/sys/bitstring.h
114–116

We don't want to prevent a future declaration of roundup2 in the user's program either. So something like this?

#ifdef roundup2
#define        _bit_roundup2 roundup2
#else
#define        _bit_roundup2(x, y)        (((x)+((y)-1))&(~((y)-1))) /* if y is powers of two */
#endif

Move to private section and update call sites to use the private macro name.

This revision now requires changes to proceed.Jun 24 2016, 4:48 PM
asomers updated this revision to Diff 17862.Jun 24 2016, 5:04 PM
asomers edited edge metadata.

Don't define a publicly visible "roundup2"

gibbs accepted this revision.Jun 24 2016, 5:06 PM
gibbs edited edge metadata.

Please fix the extra whitespace and test both with and without a roundup2 defined prior to including the header before landing.

sys/sys/bitstring.h
78

Extra newline.

This revision is now accepted and ready to land.Jun 24 2016, 5:06 PM
Closed by commit rS302180: Fix bitstring allocation on 32-bit platforms (authored by asomers). · Explain WhyJun 24 2016, 9:45 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
sys/
sys/
14 lines
tests/
sys/
sys/
12 lines

Diff 17862

View Options

sys/sys/bitstring.h

Show First 20 Lines • Show All 1,630 Lines • ▼ Show 20 Lines INP_WUNLOCK(in6p); \
{ {
struct ip6_pktopts **optp; struct ip6_pktopts **optp;
/* cannot mix with RFC2292 */ /* cannot mix with RFC2292 */
if (OPTBIT(IN6P_RFC2292)) { if (OPTBIT(IN6P_RFC2292)) {
error = EINVAL; error = EINVAL;
break; break;
} }
INP_WLOCK(in6p);
if (in6p->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
INP_WUNLOCK(in6p);
return (ECONNRESET);
}
optp = &in6p->in6p_outputopts; optp = &in6p->in6p_outputopts;
error = ip6_pcbopt(IPV6_HOPLIMIT, error = ip6_pcbopt(IPV6_HOPLIMIT,
(u_char *)&optval, sizeof(optval), (u_char *)&optval, sizeof(optval),
optp, (td != NULL) ? td->td_ucred : optp, (td != NULL) ? td->td_ucred :
NULL, uproto); NULL, uproto);
INP_WUNLOCK(in6p);
break; break;
} }
case IPV6_RECVHOPLIMIT: case IPV6_RECVHOPLIMIT:
OPTSET2292_EXCLUSIVE(IN6P_HOPLIMIT); OPTSET2292_EXCLUSIVE(IN6P_HOPLIMIT);
break; break;
case IPV6_RECVHOPOPTS: case IPV6_RECVHOPOPTS:
▲ Show 20 Lines • Show All 93 Lines • ▼ Show 20 Lines#endif
break; break;
} }
error = sooptcopyin(sopt, &optval, error = sooptcopyin(sopt, &optval,
sizeof optval, sizeof optval); sizeof optval, sizeof optval);
if (error) if (error)
break; break;
{ {
struct ip6_pktopts **optp; struct ip6_pktopts **optp;
INP_WLOCK(in6p);
if (in6p->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
INP_WUNLOCK(in6p);
return (ECONNRESET);
}
optp = &in6p->in6p_outputopts; optp = &in6p->in6p_outputopts;
error = ip6_pcbopt(optname, error = ip6_pcbopt(optname,
(u_char *)&optval, sizeof(optval), (u_char *)&optval, sizeof(optval),
optp, (td != NULL) ? td->td_ucred : optp, (td != NULL) ? td->td_ucred :
NULL, uproto); NULL, uproto);
INP_WUNLOCK(in6p);
break; break;
} }
case IPV6_2292PKTINFO: case IPV6_2292PKTINFO:
case IPV6_2292HOPLIMIT: case IPV6_2292HOPLIMIT:
case IPV6_2292HOPOPTS: case IPV6_2292HOPOPTS:
case IPV6_2292DSTOPTS: case IPV6_2292DSTOPTS:
case IPV6_2292RTHDR: case IPV6_2292RTHDR:
▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Lines#endif
* later. * later.
*/ */
error = sooptcopyin(sopt, optbuf_storage, error = sooptcopyin(sopt, optbuf_storage,
sizeof(optbuf_storage), 0); sizeof(optbuf_storage), 0);
if (error) if (error)
break; break;
optlen = sopt->sopt_valsize; optlen = sopt->sopt_valsize;
optbuf = optbuf_storage; optbuf = optbuf_storage;
INP_WLOCK(in6p);
if (in6p->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
INP_WUNLOCK(in6p);
return (ECONNRESET);
}
optp = &in6p->in6p_outputopts; optp = &in6p->in6p_outputopts;
error = ip6_pcbopt(optname, optbuf, optlen, error = ip6_pcbopt(optname, optbuf, optlen,
optp, (td != NULL) ? td->td_ucred : NULL, optp, (td != NULL) ? td->td_ucred : NULL,
uproto); uproto);
INP_WUNLOCK(in6p);
break; break;
} }
#undef OPTSET #undef OPTSET
case IPV6_MULTICAST_IF: case IPV6_MULTICAST_IF:
case IPV6_MULTICAST_HOPS: case IPV6_MULTICAST_HOPS:
case IPV6_MULTICAST_LOOP: case IPV6_MULTICAST_LOOP:
case IPV6_JOIN_GROUP: case IPV6_JOIN_GROUP:
▲ Show 20 Lines • Show All 430 Lines • ▼ Show 20 Lines
static int static int
ip6_pcbopt(int optname, u_char *buf, int len, struct ip6_pktopts **pktopt, ip6_pcbopt(int optname, u_char *buf, int len, struct ip6_pktopts **pktopt,
struct ucred *cred, int uproto) struct ucred *cred, int uproto)
{ {
struct ip6_pktopts *opt; struct ip6_pktopts *opt;
if (*pktopt == NULL) { if (*pktopt == NULL) {
*pktopt = malloc(sizeof(struct ip6_pktopts), M_IP6OPT, *pktopt = malloc(sizeof(struct ip6_pktopts), M_IP6OPT,
M_WAITOK); M_NOWAIT);
if (*pktopt == NULL)
return (ENOBUFS);
ip6_initpktopts(*pktopt); ip6_initpktopts(*pktopt);
} }
opt = *pktopt; opt = *pktopt;
return (ip6_setpktopt(optname, buf, len, opt, cred, 1, 0, uproto)); return (ip6_setpktopt(optname, buf, len, opt, cred, 1, 0, uproto));
} }
#define GET_PKTOPT_VAR(field, lenexpr) do { \ #define GET_PKTOPT_VAR(field, lenexpr) do { \
▲ Show 20 Lines • Show All 810 Lines • Show Last 20 Lines
View Options

tests/sys/sys/bitstring_test.c

Show First 20 Lines • Show All 1,630 Lines • ▼ Show 20 Lines INP_WUNLOCK(in6p); \
{ {
struct ip6_pktopts **optp; struct ip6_pktopts **optp;
/* cannot mix with RFC2292 */ /* cannot mix with RFC2292 */
if (OPTBIT(IN6P_RFC2292)) { if (OPTBIT(IN6P_RFC2292)) {
error = EINVAL; error = EINVAL;
break; break;
} }
INP_WLOCK(in6p);
if (in6p->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
INP_WUNLOCK(in6p);
return (ECONNRESET);
}
optp = &in6p->in6p_outputopts; optp = &in6p->in6p_outputopts;
error = ip6_pcbopt(IPV6_HOPLIMIT, error = ip6_pcbopt(IPV6_HOPLIMIT,
(u_char *)&optval, sizeof(optval), (u_char *)&optval, sizeof(optval),
optp, (td != NULL) ? td->td_ucred : optp, (td != NULL) ? td->td_ucred :
NULL, uproto); NULL, uproto);
INP_WUNLOCK(in6p);
break; break;
} }
case IPV6_RECVHOPLIMIT: case IPV6_RECVHOPLIMIT:
OPTSET2292_EXCLUSIVE(IN6P_HOPLIMIT); OPTSET2292_EXCLUSIVE(IN6P_HOPLIMIT);
break; break;
case IPV6_RECVHOPOPTS: case IPV6_RECVHOPOPTS:
▲ Show 20 Lines • Show All 93 Lines • ▼ Show 20 Lines#endif
break; break;
} }
error = sooptcopyin(sopt, &optval, error = sooptcopyin(sopt, &optval,
sizeof optval, sizeof optval); sizeof optval, sizeof optval);
if (error) if (error)
break; break;
{ {
struct ip6_pktopts **optp; struct ip6_pktopts **optp;
INP_WLOCK(in6p);
if (in6p->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
INP_WUNLOCK(in6p);
return (ECONNRESET);
}
optp = &in6p->in6p_outputopts; optp = &in6p->in6p_outputopts;
error = ip6_pcbopt(optname, error = ip6_pcbopt(optname,
(u_char *)&optval, sizeof(optval), (u_char *)&optval, sizeof(optval),
optp, (td != NULL) ? td->td_ucred : optp, (td != NULL) ? td->td_ucred :
NULL, uproto); NULL, uproto);
INP_WUNLOCK(in6p);
break; break;
} }
case IPV6_2292PKTINFO: case IPV6_2292PKTINFO:
case IPV6_2292HOPLIMIT: case IPV6_2292HOPLIMIT:
case IPV6_2292HOPOPTS: case IPV6_2292HOPOPTS:
case IPV6_2292DSTOPTS: case IPV6_2292DSTOPTS:
case IPV6_2292RTHDR: case IPV6_2292RTHDR:
▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Lines#endif
* later. * later.
*/ */
error = sooptcopyin(sopt, optbuf_storage, error = sooptcopyin(sopt, optbuf_storage,
sizeof(optbuf_storage), 0); sizeof(optbuf_storage), 0);
if (error) if (error)
break; break;
optlen = sopt->sopt_valsize; optlen = sopt->sopt_valsize;
optbuf = optbuf_storage; optbuf = optbuf_storage;
INP_WLOCK(in6p);
if (in6p->inp_flags & (INP_TIMEWAIT | INP_DROPPED)) {
INP_WUNLOCK(in6p);
return (ECONNRESET);
}
optp = &in6p->in6p_outputopts; optp = &in6p->in6p_outputopts;
error = ip6_pcbopt(optname, optbuf, optlen, error = ip6_pcbopt(optname, optbuf, optlen,
optp, (td != NULL) ? td->td_ucred : NULL, optp, (td != NULL) ? td->td_ucred : NULL,
uproto); uproto);
INP_WUNLOCK(in6p);
break; break;
} }
#undef OPTSET #undef OPTSET
case IPV6_MULTICAST_IF: case IPV6_MULTICAST_IF:
case IPV6_MULTICAST_HOPS: case IPV6_MULTICAST_HOPS:
case IPV6_MULTICAST_LOOP: case IPV6_MULTICAST_LOOP:
case IPV6_JOIN_GROUP: case IPV6_JOIN_GROUP:
▲ Show 20 Lines • Show All 430 Lines • ▼ Show 20 Lines
static int static int
ip6_pcbopt(int optname, u_char *buf, int len, struct ip6_pktopts **pktopt, ip6_pcbopt(int optname, u_char *buf, int len, struct ip6_pktopts **pktopt,
struct ucred *cred, int uproto) struct ucred *cred, int uproto)
{ {
struct ip6_pktopts *opt; struct ip6_pktopts *opt;
if (*pktopt == NULL) { if (*pktopt == NULL) {
*pktopt = malloc(sizeof(struct ip6_pktopts), M_IP6OPT, *pktopt = malloc(sizeof(struct ip6_pktopts), M_IP6OPT,
M_WAITOK); M_NOWAIT);
if (*pktopt == NULL)
return (ENOBUFS);
ip6_initpktopts(*pktopt); ip6_initpktopts(*pktopt);
} }
opt = *pktopt; opt = *pktopt;
return (ip6_setpktopt(optname, buf, len, opt, cred, 1, 0, uproto)); return (ip6_setpktopt(optname, buf, len, opt, cred, 1, 0, uproto));
} }
#define GET_PKTOPT_VAR(field, lenexpr) do { \ #define GET_PKTOPT_VAR(field, lenexpr) do { \
▲ Show 20 Lines • Show All 810 Lines • Show Last 20 Lines