| Message ID | 1388427307-8691-5-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, 2013-12-30 at 19:15 +0100, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > Ether_addr_equal_64bits is more efficient than ether_addr_equal, and can be > used when each argument is an array within a structure that contains at > least two bytes of data beyond the array. > > The structures involved are: > iwl_rxon_cmd defined in drivers/net/wireless/iwlwifi/dvm/commands.h and > ieee80211_hdr defined in include/linux/ieee80211.h > > This was done using Coccinelle (http://coccinelle.lip6.fr/). Seems to be missing an "iwlwifi:" or so prefix, but I guess we can add it when we take the patch ... Is there any way we could catch (sparse, or some other script?) that struct reorganising won't break the condition needed ("within a structure that contains at least two more bytes")? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Seems to be missing an "iwlwifi:" or so prefix, but I guess we can add > it when we take the patch ... Sorry. Not sure why that happened. I'll look into it. > Is there any way we could catch (sparse, or some other script?) that > struct reorganising won't break the condition needed ("within a > structure that contains at least two more bytes")? What kind of reorganizing could happen? Do you mean that the programmer might do at some time in the future, or something the compiler might do? julia -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: > > Is there any way we could catch (sparse, or some other script?) that > > struct reorganising won't break the condition needed ("within a > > structure that contains at least two more bytes")? > > What kind of reorganizing could happen? Do you mean that the programmer > might do at some time in the future, or something the compiler might do? I'm just thinking of a programmer, e.g. changing a struct like this: struct foo { u8 addr[ETH_ALEN]; - u16 dummy; }; for example. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 30 Dec 2013, Johannes Berg wrote: > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: > > > Is there any way we could catch (sparse, or some other script?) that > > > struct reorganising won't break the condition needed ("within a > > > structure that contains at least two more bytes")? > > > > What kind of reorganizing could happen? Do you mean that the programmer > > might do at some time in the future, or something the compiler might do? > > I'm just thinking of a programmer, e.g. changing a struct like this: > > struct foo { > u8 addr[ETH_ALEN]; > - u16 dummy; > }; > > for example. That is easily resolved by: struct foo { u8 addr[ETH_ALEN]; u16 required_padding; /* do not remove upon pain of death */ }; Preferably with a comment nearby explaining just why that padding is so important in the first place. Unfortunately, we do have a lot of code with unexplained padding that one should never remove, and it is often nowhere nearly as obvious as stray elements with idiotic non-explanatory names in a structure. One example is the via-rng hw_random driver, where the buffer used by the "xstore" via-specific instriction *MUST* be longer than what the "xstore" instruction is documented to require due to CPU errata (you ask it to write from 1 to 8 bytes, and it may end up writing 16 bytes). The xstore buffer is an array that was made long enough to avoid the errata, but that fact is documented only in a git commit message.
On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote: > On Mon, 30 Dec 2013, Johannes Berg wrote: > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: > > > > Is there any way we could catch (sparse, or some other script?) that > > > > struct reorganising won't break the condition needed ("within a > > > > structure that contains at least two more bytes")? > > > > > > What kind of reorganizing could happen? Do you mean that the programmer > > > might do at some time in the future, or something the compiler might do? > > > > I'm just thinking of a programmer, e.g. changing a struct like this: > > > > struct foo { > > u8 addr[ETH_ALEN]; > > - u16 dummy; > > }; > > > > for example. > > That is easily resolved by: > > struct foo { > u8 addr[ETH_ALEN]; > u16 required_padding; /* do not remove upon pain of death */ > }; That'd be a stupid waste of struct space. If anything, there should be *only* a comment saying that at least two bytes are needed - I'd still prefer an automated check. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-12-31 at 00:13 +0100, Johannes Berg wrote: > On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote: > > On Mon, 30 Dec 2013, Johannes Berg wrote: > > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: > > > > > Is there any way we could catch (sparse, or some other script?) that > > > > > struct reorganising won't break the condition needed ("within a > > > > > structure that contains at least two more bytes")? > > > > > > > > What kind of reorganizing could happen? Do you mean that the programmer > > > > might do at some time in the future, or something the compiler might do? > > > > > > I'm just thinking of a programmer, e.g. changing a struct like this: > > > > > > struct foo { > > > u8 addr[ETH_ALEN]; > > > - u16 dummy; > > > }; I don't know of a way to catch that. Anyone else? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 31, 2013 at 1:13 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote: > > On Mon, 30 Dec 2013, Johannes Berg wrote: > > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: > > > > > Is there any way we could catch (sparse, or some other script?) that > > > > > struct reorganising won't break the condition needed ("within a > > > > > structure that contains at least two more bytes")? > > > > > > > > What kind of reorganizing could happen? Do you mean that the programmer > > > > might do at some time in the future, or something the compiler might do? > > > > > > I'm just thinking of a programmer, e.g. changing a struct like this: > > > > > > struct foo { > > > u8 addr[ETH_ALEN]; > > > - u16 dummy; > > > }; > > > > > > for example. > > > > That is easily resolved by: > > > > struct foo { > > u8 addr[ETH_ALEN]; > > u16 required_padding; /* do not remove upon pain of death */ > > }; > > That'd be a stupid waste of struct space. If anything, there should be > *only* a comment saying that at least two bytes are needed - I'd still > prefer an automated check. > Frankly I am not sure I like the patch. This flow is not a fast path at all. While I don't really care for the waste in iwlwifi (because there isn't), I don't see the real point is make the code more sensitive to changes to earn basically nothing. This flow happens only upon association which means a few times an hour maybe... The only advantage I see here is that people like me who don't always have a chance to read / write much code outside their little tiny boring driver get to know about this kind of things. So, from an educational point of view - this is cool. But education is one thing, and the code is another. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > I'm just thinking of a programmer, e.g. changing a struct like this: > > > > > > > > struct foo { > > > > u8 addr[ETH_ALEN]; > > > > - u16 dummy; > > > > }; > > I don't know of a way to catch that. > Anyone else? Well, one could have a semantic patch that checks for that. But the problem is that it is very slow, and it only covers the cases that I can transform automatically, which currently means no pointers, only explicit arrays. On the other hand, I am finding the structure definition, so I can easily update the structure definition with an appropriate comment. struct foo { u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */ u16 dummy; }; Unfortunately it is kind of verbose. Could there be an attribute? That could even easily be checked. julia -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/30/2013 10:32 PM, Julia Lawall wrote: >>>>> I'm just thinking of a programmer, e.g. changing a struct like this: >>>>> >>>>> struct foo { >>>>> u8 addr[ETH_ALEN]; >>>>> - u16 dummy; >>>>> }; >> >> I don't know of a way to catch that. >> Anyone else? > > Well, one could have a semantic patch that checks for that. But the > problem is that it is very slow, and it only covers the cases that I can > transform automatically, which currently means no pointers, only explicit > arrays. > > On the other hand, I am finding the structure definition, so I can easily > update the structure definition with an appropriate comment. > > struct foo { > u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */ > u16 dummy; > }; > > Unfortunately it is kind of verbose. Could there be an attribute? That > could even easily be checked. Can you not just add a build-time macro to check that sizeof(foo) >= 8 for each of these struct foos? Or, is it required that the dummy field be there and be not used by anything else? Thanks, Ben > > julia > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Tue, 31 Dec 2013, Ben Greear wrote: > On 12/30/2013 10:32 PM, Julia Lawall wrote: > > > > > > I'm just thinking of a programmer, e.g. changing a struct like this: > > > > > > > > > > > > struct foo { > > > > > > u8 addr[ETH_ALEN]; > > > > > > - u16 dummy; > > > > > > }; > > > > > > I don't know of a way to catch that. > > > Anyone else? > > > > Well, one could have a semantic patch that checks for that. But the > > problem is that it is very slow, and it only covers the cases that I can > > transform automatically, which currently means no pointers, only explicit > > arrays. > > > > On the other hand, I am finding the structure definition, so I can easily > > update the structure definition with an appropriate comment. > > > > struct foo { > > u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */ > > u16 dummy; > > }; > > > > Unfortunately it is kind of verbose. Could there be an attribute? That > > could even easily be checked. > > Can you not just add a build-time macro to check that sizeof(foo) >= 8 > for each of these struct foos? Or, is it required that the dummy field > be there and be not used by anything else? It doesn't matter what the field is used for. The problem is that is it necessary to ensure a property of the position of addr within the structure. It has to have at least 16 bytes after it. But maybe something with sizeof(foo) and offset_of would do? Could the macro be put near the declaration of the structure somehow? julia -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/31/2013 08:09 AM, Julia Lawall wrote: > > > On Tue, 31 Dec 2013, Ben Greear wrote: > >> On 12/30/2013 10:32 PM, Julia Lawall wrote: >>>>>>> I'm just thinking of a programmer, e.g. changing a struct like this: >>>>>>> >>>>>>> struct foo { >>>>>>> u8 addr[ETH_ALEN]; >>>>>>> - u16 dummy; >>>>>>> }; >>>> >>>> I don't know of a way to catch that. >>>> Anyone else? >>> >>> Well, one could have a semantic patch that checks for that. But the >>> problem is that it is very slow, and it only covers the cases that I can >>> transform automatically, which currently means no pointers, only explicit >>> arrays. >>> >>> On the other hand, I am finding the structure definition, so I can easily >>> update the structure definition with an appropriate comment. >>> >>> struct foo { >>> u8 addr[ETH_ALEN]; /* must be followed by two bytes in the structure */ >>> u16 dummy; >>> }; >>> >>> Unfortunately it is kind of verbose. Could there be an attribute? That >>> could even easily be checked. >> >> Can you not just add a build-time macro to check that sizeof(foo) >= 8 >> for each of these struct foos? Or, is it required that the dummy field >> be there and be not used by anything else? > > It doesn't matter what the field is used for. The problem is that is it > necessary to ensure a property of the position of addr within the > structure. It has to have at least 16 bytes after it. You mean 16 bits? > > But maybe something with sizeof(foo) and offset_of would do? > > Could the macro be put near the declaration of the structure somehow? I think that would work, but do not know all of the details of such macros, so it's possible there is some catch. If nothing else, then some run-time code that calculates the offset off and asserts if it is broken in module initialization or similar might be good enough. Thanks, Ben
On Tue, 31 Dec 2013, Ben Greear wrote: > On 12/31/2013 08:09 AM, Julia Lawall wrote: > > > > > > On Tue, 31 Dec 2013, Ben Greear wrote: > > > > > On 12/30/2013 10:32 PM, Julia Lawall wrote: > > > > > > > > I'm just thinking of a programmer, e.g. changing a struct like > > > > > > > > this: > > > > > > > > > > > > > > > > struct foo { > > > > > > > > u8 addr[ETH_ALEN]; > > > > > > > > - u16 dummy; > > > > > > > > }; > > > > > > > > > > I don't know of a way to catch that. > > > > > Anyone else? > > > > > > > > Well, one could have a semantic patch that checks for that. But the > > > > problem is that it is very slow, and it only covers the cases that I can > > > > transform automatically, which currently means no pointers, only > > > > explicit > > > > arrays. > > > > > > > > On the other hand, I am finding the structure definition, so I can > > > > easily > > > > update the structure definition with an appropriate comment. > > > > > > > > struct foo { > > > > u8 addr[ETH_ALEN]; /* must be followed by two bytes in the > > > > structure */ > > > > u16 dummy; > > > > }; > > > > > > > > Unfortunately it is kind of verbose. Could there be an attribute? That > > > > could even easily be checked. > > > > > > Can you not just add a build-time macro to check that sizeof(foo) >= 8 > > > for each of these struct foos? Or, is it required that the dummy field > > > be there and be not used by anything else? > > > > It doesn't matter what the field is used for. The problem is that is it > > necessary to ensure a property of the position of addr within the > > structure. It has to have at least 16 bytes after it. > > You mean 16 bits? Oops, yes. 16 bits. > > > > But maybe something with sizeof(foo) and offset_of would do? > > > > Could the macro be put near the declaration of the structure somehow? > > I think that would work, but do not know all of the details of such > macros, so it's possible there is some catch. > > If nothing else, then some run-time code that calculates the offset off > and asserts if it is broken in module initialization or similar might > be good enough. Could be OK. Something right in or after the structure declaration would be nicest. julia > > Thanks, > Ben > > -- > Ben Greear <greearb@candelatech.com> > Candela Technologies Inc http://www.candelatech.com > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Is there any way to get sizeof evaluated at build time? julia -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-01-06 at 09:48 +0100, Julia Lawall wrote:
> Is there any way to get sizeof evaluated at build time?
I'm confused a bit by what you want to accomplish.
Except for variable length arrays, isn't sizeof always
evaluated at build time?
6.5.3.4 The sizeof operator
Constraints
[]
2 The sizeof operator yields the size (in bytes) of its operand, which may be an
expression or the parenthesized name of a type. The size is determined from the type of
the operand. The result is an integer. If the type of the operand is a variable length array
type, the operand is evaluated; otherwise, the operand is not evaluated and the result is an
integer constant.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 6 Jan 2014, Joe Perches wrote: > On Mon, 2014-01-06 at 09:48 +0100, Julia Lawall wrote: > > Is there any way to get sizeof evaluated at build time? > > I'm confused a bit by what you want to accomplish. To check that a field that is an argument od ether_addr_equal_64bits is a sufficient distance from the end of the structure. > Except for variable length arrays, isn't sizeof always > evaluated at build time? OK, the question was expressed badly. Is there any way to use the value to trigger an action at build time? The only way I kow to trigger an action is with #error, but #error is processed by cpp, which doesn't know about the size of types. julia > 6.5.3.4 The sizeof operator > Constraints > [] > 2 The sizeof operator yields the size (in bytes) of its operand, which may be an > expression or the parenthesized name of a type. The size is determined from the type of > the operand. The result is an integer. If the type of the operand is a variable length array > type, the operand is evaluated; otherwise, the operand is not evaluated and the result is an > integer constant. > > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-12-31 at 17:40 +0100, Julia Lawall wrote: > > If nothing else, then some run-time code that calculates the offset off > > and asserts if it is broken in module initialization or similar might > > be good enough. > > Could be OK. Something right in or after the structure declaration would > be nicest. I don't think you can put a BUILD_BUG_ON() into the structure declaration (it's code, not declarations), but I think you could just put BUILD_BUG_ON(sizeof(struct foo) - offsetof(struct foo, addr) < 8); with the user(s?) and that should catch the scenario I was worrying about? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-01-06 at 10:04 +0100, Julia Lawall wrote: > OK, the question was expressed badly. Is there any way to use the value > to trigger an action at build time? The only way I kow to trigger an > action is with #error, but #error is processed by cpp, which doesn't know > about the size of types. That's exactly BUILD_BUG_ON(), I believe. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 6 Jan 2014, Johannes Berg wrote: > On Tue, 2013-12-31 at 17:40 +0100, Julia Lawall wrote: > > > > If nothing else, then some run-time code that calculates the offset off > > > and asserts if it is broken in module initialization or similar might > > > be good enough. > > > > Could be OK. Something right in or after the structure declaration would > > be nicest. > > I don't think you can put a BUILD_BUG_ON() into the structure > declaration (it's code, not declarations), but I think you could just > put > > BUILD_BUG_ON(sizeof(struct foo) - offsetof(struct foo, addr) < 8); > > with the user(s?) and that should catch the scenario I was worrying > about? OK, thanks. That is what I had in mind. But I was hoping to be able to put it with the structure. Perhaps there is a way to make a macro that expands to a dummy function that contains the BUILD_BUG_ON? But I guess that would waste space? I think that 8 should be 16? julia -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 6 Jan 2014, Johannes Berg wrote: > On Mon, 2014-01-06 at 10:04 +0100, Julia Lawall wrote: > > > OK, the question was expressed badly. Is there any way to use the value > > to trigger an action at build time? The only way I kow to trigger an > > action is with #error, but #error is processed by cpp, which doesn't know > > about the size of types. > > That's exactly BUILD_BUG_ON(), I believe. Maybe BUILD_BUG_ON_ZERO? It's goal seems to be to return an integer, which could then be used as an array size in a dummy structure. The structure declaration would not generate any code. The structure would have to have a name, thoough. julia -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 31, 2013 at 7:26 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote: > On Tue, Dec 31, 2013 at 1:13 AM, Johannes Berg > <johannes@sipsolutions.net> wrote: >> >> On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote: >> > On Mon, 30 Dec 2013, Johannes Berg wrote: >> > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: >> > > > > Is there any way we could catch (sparse, or some other script?) that >> > > > > struct reorganising won't break the condition needed ("within a >> > > > > structure that contains at least two more bytes")? >> > > > >> > > > What kind of reorganizing could happen? Do you mean that the programmer >> > > > might do at some time in the future, or something the compiler might do? >> > > >> > > I'm just thinking of a programmer, e.g. changing a struct like this: >> > > >> > > struct foo { >> > > u8 addr[ETH_ALEN]; >> > > - u16 dummy; >> > > }; >> > > >> > > for example. >> > >> > That is easily resolved by: >> > >> > struct foo { >> > u8 addr[ETH_ALEN]; >> > u16 required_padding; /* do not remove upon pain of death */ >> > }; Adding the u16 also changes the alignment of the whole struct. So it may cost one additional byte _in front of_ the struct. <asking-stupid-question-see-answer-below> While you're at it, why not just making a new 64-bit aligned type for Ethernet addresses, so it'll also work for !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS? </asking-stupid-question-see-answer-below> >> That'd be a stupid waste of struct space. If anything, there should be >> *only* a comment saying that at least two bytes are needed - I'd still >> prefer an automated check. > > Frankly I am not sure I like the patch. This flow is not a fast path I also don't like it. To me this sounds like wasting space for nothing. BTW, would it be that more expensive to always do a 32+16 bit comparison? > at all. While I don't really care for the waste in iwlwifi (because > there isn't), I don't see the real point is make the code more > sensitive to changes to earn basically nothing. Thanks to this discussion, my eye fell on: static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2) { const u16 *a = (const u16 *) addr1; const u16 *b = (const u16 *) addr2; BUILD_BUG_ON(ETH_ALEN != 6); return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0; } What if addr1 or addr2 are odd, and this is running on an architecture that doesn't support unaligned accesses at all?? Have we been lucky forever? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 6 Jan 2014, Geert Uytterhoeven wrote: > On Tue, Dec 31, 2013 at 7:26 AM, Emmanuel Grumbach <egrumbach@gmail.com> wrote: > > On Tue, Dec 31, 2013 at 1:13 AM, Johannes Berg > > <johannes@sipsolutions.net> wrote: > >> > >> On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote: > >> > On Mon, 30 Dec 2013, Johannes Berg wrote: > >> > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: > >> > > > > Is there any way we could catch (sparse, or some other script?) that > >> > > > > struct reorganising won't break the condition needed ("within a > >> > > > > structure that contains at least two more bytes")? > >> > > > > >> > > > What kind of reorganizing could happen? Do you mean that the programmer > >> > > > might do at some time in the future, or something the compiler might do? > >> > > > >> > > I'm just thinking of a programmer, e.g. changing a struct like this: > >> > > > >> > > struct foo { > >> > > u8 addr[ETH_ALEN]; > >> > > - u16 dummy; > >> > > }; > >> > > > >> > > for example. > >> > > >> > That is easily resolved by: > >> > > >> > struct foo { > >> > u8 addr[ETH_ALEN]; > >> > u16 required_padding; /* do not remove upon pain of death */ > >> > }; > > Adding the u16 also changes the alignment of the whole struct. So it may > cost one additional byte _in front of_ the struct. > > <asking-stupid-question-see-answer-below> > While you're at it, why not just making a new 64-bit aligned type for > Ethernet addresses, so it'll also work for > !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS? > </asking-stupid-question-see-answer-below> > > >> That'd be a stupid waste of struct space. If anything, there should be > >> *only* a comment saying that at least two bytes are needed - I'd still > >> prefer an automated check. > > > > Frankly I am not sure I like the patch. This flow is not a fast path > > I also don't like it. To me this sounds like wasting space for nothing. > BTW, would it be that more expensive to always do a 32+16 bit > comparison? No space is wasted by the patch. The patch is only generated in the case where there is currently enough space. julia > > at all. While I don't really care for the waste in iwlwifi (because > > there isn't), I don't see the real point is make the code more > > sensitive to changes to earn basically nothing. > > Thanks to this discussion, my eye fell on: > > static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2) > { > const u16 *a = (const u16 *) addr1; > const u16 *b = (const u16 *) addr2; > > BUILD_BUG_ON(ETH_ALEN != 6); > return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0; > } > > What if addr1 or addr2 are odd, and this is running on an architecture that > doesn't support unaligned accesses at all?? Have we been lucky forever? > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-01-06 at 10:09 +0100, Julia Lawall wrote: > > BUILD_BUG_ON(sizeof(struct foo) - offsetof(struct foo, addr) < 8); > > > > with the user(s?) and that should catch the scenario I was worrying > > about? > > OK, thanks. That is what I had in mind. But I was hoping to be able to > put it with the structure. Right - you might be able to do that with BUILD_BUG_ON_ZERO() as you pointed out, I haven't looked at these macros in a while. > Perhaps there is a way to make a macro that > expands to a dummy function that contains the BUILD_BUG_ON? But I guess > that would waste space? > > I think that 8 should be 16? No, that should be ETH_ALEN+2 really, I guess - it's not taking into account the size of the address member itself at all. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 31, 2013 at 12:13:08AM +0100, Johannes Berg wrote: > On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote: > > On Mon, 30 Dec 2013, Johannes Berg wrote: > > > On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote: > > > > > Is there any way we could catch (sparse, or some other script?) that > > > > > struct reorganising won't break the condition needed ("within a > > > > > structure that contains at least two more bytes")? > > > > > > > > What kind of reorganizing could happen? Do you mean that the programmer > > > > might do at some time in the future, or something the compiler might do? > > > > > > I'm just thinking of a programmer, e.g. changing a struct like this: > > > > > > struct foo { > > > u8 addr[ETH_ALEN]; > > > - u16 dummy; > > > }; > > > > > > for example. > > > > That is easily resolved by: > > > > struct foo { > > u8 addr[ETH_ALEN]; > > u16 required_padding; /* do not remove upon pain of death */ > > }; > > That'd be a stupid waste of struct space. If anything, there should be > *only* a comment saying that at least two bytes are needed - I'd still > prefer an automated check. > This is the sort of thing where Smatch is probably the right tool. I'll let you know how it turns out. My guess is that it would be rare to run into this bug in real life. Most structs have 4 or 8 byte alignment and most times the addr is not at the end of the struct. But I also agree that this function should only be used in a fast path. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-01-06 at 10:24 +0100, Geert Uytterhoeven wrote: > Thanks to this discussion, my eye fell on: > > static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2) > { > const u16 *a = (const u16 *) addr1; > const u16 *b = (const u16 *) addr2; > > BUILD_BUG_ON(ETH_ALEN != 6); > return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0; > } > > What if addr1 or addr2 are odd, and this is running on an architecture that > doesn't support unaligned accesses at all?? Have we been lucky forever? This function always had the guarantee of u16 alignment. No protocol ever included an ether address at an odd alignment, and drivers always make sure a frame is at least 2-byte aligned. Its kind of obvious for networking people, Stephen did not mention this property in commit 360ac8e2f1a38c34 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
We're worried about reading beyond the end of the array and it's a heap
allocation and the last char of the eth addr is the last byte of the
page. This causes an oops.
It's almost impossible to hit that bug.
1) You would have to have the eth addr at the end of the array.
2) It would have to be a packed struct.
3) The struct size would have to be a multiple of 4 because otherwise we
can't put it at the end of the page.
4) It would need to be allocated on the heap.
You add all those up which is pretty rare so I wasn't able to find
anything like that. Then you have to get extremely unlucky.
The closest thing I could find were a couple places like like:
static struct mac_addr null_mac_addr = { { 0, 0, 0, 0, 0, 0 } };
It meets criteria 1 and 2 but not 3 and 4.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/iwlwifi/dvm/rxon.c b/drivers/net/wireless/iwlwifi/dvm/rxon.c index d7ce2f1..0439dd5 100644 --- a/drivers/net/wireless/iwlwifi/dvm/rxon.c +++ b/drivers/net/wireless/iwlwifi/dvm/rxon.c @@ -875,10 +875,10 @@ static int iwl_full_rxon_required(struct iwl_priv *priv, /* These items are only settable from the full RXON command */ CHK(!iwl_is_associated_ctx(ctx)); - CHK(!ether_addr_equal(staging->bssid_addr, active->bssid_addr)); - CHK(!ether_addr_equal(staging->node_addr, active->node_addr)); - CHK(!ether_addr_equal(staging->wlap_bssid_addr, - active->wlap_bssid_addr)); + CHK(!ether_addr_equal_64bits(staging->bssid_addr, active->bssid_addr)); + CHK(!ether_addr_equal_64bits(staging->node_addr, active->node_addr)); + CHK(!ether_addr_equal_64bits(staging->wlap_bssid_addr, + active->wlap_bssid_addr)); CHK_NEQ(staging->dev_type, active->dev_type); CHK_NEQ(staging->channel, active->channel); CHK_NEQ(staging->air_propagation, active->air_propagation); diff --git a/drivers/net/wireless/iwlwifi/dvm/rx.c b/drivers/net/wireless/iwlwifi/dvm/rx.c index d71776d..d18570b 100644 --- a/drivers/net/wireless/iwlwifi/dvm/rx.c +++ b/drivers/net/wireless/iwlwifi/dvm/rx.c @@ -780,8 +780,8 @@ static void iwlagn_pass_packet_to_mac80211(struct iwl_priv *priv, */ if (unlikely(ieee80211_is_beacon(fc) && priv->passive_no_rx)) { for_each_context(priv, ctx) { - if (!ether_addr_equal(hdr->addr3, - ctx->active.bssid_addr)) + if (!ether_addr_equal_64bits(hdr->addr3, + ctx->active.bssid_addr)) continue; iwlagn_lift_passive_no_rx(priv); }