[PATCH v2] net: bridge: add compile-time assert for cb struct size

[PATCH v2] net: bridge: add compile-time assert for cb struct size

[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

RE: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[David Laight]

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

Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[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.

RE: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[David Laight]

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

Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[Eric Dumazet]

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.

Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[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.

Besides that, this macro is also buggy, my totally untrained lisp-eye tells
me that you got your brackets wrong.

Cheers,
Daniel

RE: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[David Laight]

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

Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[Eyal Birger]

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.

Re: [PATCH v2] net: bridge: add compile-time assert for cb struct size

[David Miller]

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.