* [PATCH v2] net: bridge: add compile-time assert for cb struct size
@ 2015-03-03 12:53 Florian Westphal
2015-03-03 13:50 ` David Laight
2015-03-03 19:07 ` David Miller
0 siblings, 2 replies; 9+ messages in thread
From: Florian Westphal @ 2015-03-03 12:53 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
make build fail if structure no longer fits into ->cb storage.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
Change since v1: use FIELD_SIZEOF
checkpatch complains about >80 chars, but line break is ugly here.
diff --git a/net/bridge/br.c b/net/bridge/br.c
index fb57ab6..02c24cf 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -190,6 +190,8 @@ static int __init br_init(void)
{
int err;
+ BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
+
err = stp_proto_register(&br_stp_proto);
if (err < 0) {
pr_err("bridge: can't register sap for STP\n");
--
2.0.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 12:53 [PATCH v2] net: bridge: add compile-time assert for cb struct size Florian Westphal
@ 2015-03-03 13:50 ` David Laight
2015-03-03 14:07 ` Eric Dumazet
2015-03-03 14:28 ` Daniel Borkmann
2015-03-03 19:07 ` David Miller
1 sibling, 2 replies; 9+ messages in thread
From: David Laight @ 2015-03-03 13:50 UTC (permalink / raw)
To: 'Florian Westphal', netdev
From: Florian Westphal
> make build fail if structure no longer fits into ->cb storage.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> Change since v1: use FIELD_SIZEOF
>
> checkpatch complains about >80 chars, but line break is ugly here.
>
> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index fb57ab6..02c24cf 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -190,6 +190,8 @@ static int __init br_init(void)
> {
> int err;
>
> + BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
> +
What about using something based on:
#define GET_CB(skb, type) ((type *)((skb)->cb + (sizeof (char[(sizeof (type) <= sizeof (skb->cb)) ? 1 : -1] - 1)))
to access skb->sb?
Then all invalid uses are likely to be picked up.
David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 13:50 ` David Laight
@ 2015-03-03 14:07 ` Eric Dumazet
2015-03-03 14:15 ` David Laight
2015-03-03 14:28 ` Daniel Borkmann
1 sibling, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2015-03-03 14:07 UTC (permalink / raw)
To: David Laight; +Cc: 'Florian Westphal', netdev
On Tue, 2015-03-03 at 13:50 +0000, David Laight wrote:
> From: Florian Westphal
> > make build fail if structure no longer fits into ->cb storage.
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> > Change since v1: use FIELD_SIZEOF
> >
> > checkpatch complains about >80 chars, but line break is ugly here.
> >
> > diff --git a/net/bridge/br.c b/net/bridge/br.c
> > index fb57ab6..02c24cf 100644
> > --- a/net/bridge/br.c
> > +++ b/net/bridge/br.c
> > @@ -190,6 +190,8 @@ static int __init br_init(void)
> > {
> > int err;
> >
> > + BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
> > +
>
> What about using something based on:
> #define GET_CB(skb, type) ((type *)((skb)->cb + (sizeof (char[(sizeof (type) <= sizeof (skb->cb)) ? 1 : -1] - 1)))
> to access skb->sb?
>
> Then all invalid uses are likely to be picked up.
We want a build error, not a runtime one.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 14:07 ` Eric Dumazet
@ 2015-03-03 14:15 ` David Laight
2015-03-03 14:33 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2015-03-03 14:15 UTC (permalink / raw)
To: 'Eric Dumazet'; +Cc: 'Florian Westphal', netdev
From: Eric Dumazet
> On Tue, 2015-03-03 at 13:50 +0000, David Laight wrote:
> > From: Florian Westphal
> > > make build fail if structure no longer fits into ->cb storage.
> > >
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > > Change since v1: use FIELD_SIZEOF
> > >
> > > checkpatch complains about >80 chars, but line break is ugly here.
> > >
> > > diff --git a/net/bridge/br.c b/net/bridge/br.c
> > > index fb57ab6..02c24cf 100644
> > > --- a/net/bridge/br.c
> > > +++ b/net/bridge/br.c
> > > @@ -190,6 +190,8 @@ static int __init br_init(void)
> > > {
> > > int err;
> > >
> > > + BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
> > > +
> >
> > What about using something based on:
> > #define GET_CB(skb, type) ((type *)((skb)->cb + (sizeof (char[(sizeof (type) <= sizeof (skb->cb)) ? 1 : -1] - 1)))
> > to access skb->sb?
> >
> > Then all invalid uses are likely to be picked up.
>
> We want a build error, not a runtime one.
That is a build error.
David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 13:50 ` David Laight
2015-03-03 14:07 ` Eric Dumazet
@ 2015-03-03 14:28 ` Daniel Borkmann
2015-03-03 14:44 ` David Laight
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2015-03-03 14:28 UTC (permalink / raw)
To: David Laight, 'Florian Westphal', netdev
On 03/03/2015 02:50 PM, David Laight wrote:
...
>> + BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
>> +
>
> What about using something based on:
> #define GET_CB(skb, type) ((type *)((skb)->cb + (sizeof (char[(sizeof (type) <= sizeof (skb->cb)) ? 1 : -1] - 1)))
> to access skb->sb?
That would require to change all skb->cb[] call-sites everywhere.
Besides that, this macro is also buggy, my totally untrained lisp-eye tells
me that you got your brackets wrong.
Cheers,
Daniel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 14:15 ` David Laight
@ 2015-03-03 14:33 ` Eric Dumazet
0 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2015-03-03 14:33 UTC (permalink / raw)
To: David Laight; +Cc: 'Florian Westphal', netdev
On Tue, 2015-03-03 at 14:15 +0000, David Laight wrote:
> That is a build error.
Yes, but we want a build error that this obvious to understand.
Like :
BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
Please send a tested patch so that we can compare the solutions.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 14:28 ` Daniel Borkmann
@ 2015-03-03 14:44 ` David Laight
2015-03-03 14:51 ` Eyal Birger
0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2015-03-03 14:44 UTC (permalink / raw)
To: 'Daniel Borkmann', 'Florian Westphal', netdev
From: Daniel Borkmann
> On 03/03/2015 02:50 PM, David Laight wrote:
> ...
> >> + BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
> >> +
> >
> > What about using something based on:
> > #define GET_CB(skb, type) ((type *)((skb)->cb + (sizeof (char[(sizeof (type) <= sizeof (skb->cb)) ? 1 : -1] - 1)))
> > to access skb->sb?
>
> That would require to change all skb->cb[] call-sites everywhere.
Well, only once for each type.
As opposed to adding an open coded check at all of them.
Clearly if the majority of sites have been changed then the 'cb' field member can
be renamed to pick up the stragglers. I wouldn't suggest doing that immediately.
> Besides that, this macro is also buggy, my totally untrained lisp-eye tells
> me that you got your brackets wrong.
I did say 'based on'....
It is also probably possible to get the name of the type into the error message.
Although that might require it always be a 'struct'.
David
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 14:44 ` David Laight
@ 2015-03-03 14:51 ` Eyal Birger
0 siblings, 0 replies; 9+ messages in thread
From: Eyal Birger @ 2015-03-03 14:51 UTC (permalink / raw)
To: David Laight; +Cc: Daniel Borkmann, Florian Westphal, netdev
On Tue, Mar 3, 2015 at 4:44 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Daniel Borkmann
>> On 03/03/2015 02:50 PM, David Laight wrote:
>> ...
>> >> + BUILD_BUG_ON(sizeof(struct br_input_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
>> >> +
>> >
>> > What about using something based on:
>> > #define GET_CB(skb, type) ((type *)((skb)->cb + (sizeof (char[(sizeof (type) <= sizeof (skb->cb)) ? 1 : -1] - 1)))
>> > to access skb->sb?
>>
>> That would require to change all skb->cb[] call-sites everywhere.
>
> Well, only once for each type.
Note that there are cases where skb->cb[] used size is greater than
the associated
type.
For example, in packet_rcv() a full hw address is stored in skb->cb[], exceeding
the packet_skb_cb struct size. This is also true for can/raw.c (in
raw_flags()), though
in that specific case the skb->cb[] use could have been wrapped in a struct.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size
2015-03-03 12:53 [PATCH v2] net: bridge: add compile-time assert for cb struct size Florian Westphal
2015-03-03 13:50 ` David Laight
@ 2015-03-03 19:07 ` David Miller
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2015-03-03 19:07 UTC (permalink / raw)
To: fw; +Cc: netdev
From: Florian Westphal <fw@strlen.de>
Date: Tue, 3 Mar 2015 13:53:31 +0100
> make build fail if structure no longer fits into ->cb storage.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
Applied, thanks Florian.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-03-03 19:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-03 12:53 [PATCH v2] net: bridge: add compile-time assert for cb struct size Florian Westphal
2015-03-03 13:50 ` David Laight
2015-03-03 14:07 ` Eric Dumazet
2015-03-03 14:15 ` David Laight
2015-03-03 14:33 ` Eric Dumazet
2015-03-03 14:28 ` Daniel Borkmann
2015-03-03 14:44 ` David Laight
2015-03-03 14:51 ` Eyal Birger
2015-03-03 19:07 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.