Re: ip_rcv_finish() NULL pointer kernel panic

Re: ip_rcv_finish() NULL pointer kernel panic

[Eric Dumazet]

On Thu, 2017-01-26 at 09:32 -0600, Roy Keene wrote:
> This bug appears to have existed for a long time:
> 
>  	https://www.spinics.net/lists/netdev/msg222459.html
> 
>  	http://www.kernelhub.org/?p=2&msg=823752
> 
> Though possibly with different things not setting the "input" function 
> pointer in the "struct dst_entry".
> 
> include/net/dst.h:
>   496 static inline int dst_input(struct sk_buff *skb) {
>   498         return skb_dst(skb)->input(skb);
>   499 }
> 
> Is there any reason not to check to see if this pointer is NULL before 
> blindly calling it ?

Sure. It can not be NULL at this point.

Just look at the code in ip_rcv_finish() :

It first make sure to get a valid dst.

Something is broken, probably in bridge netfilter code.

I suspect the dst here points to &br->fake_rtable, and this is not
expected.

br_drop_fake_rtable() should have been called somewhere ...

Re: ip_rcv_finish() NULL pointer kernel panic

[Roy Keene]

> On Thu, 2017-01-26 at 09:32 -0600, Roy Keene wrote:
>> This bug appears to have existed for a long time:
>>
>>  	https://www.spinics.net/lists/netdev/msg222459.html
>>
>>  	http://www.kernelhub.org/?p=2&msg=823752
>>
>> Though possibly with different things not setting the "input" function
>> pointer in the "struct dst_entry".
>>
>> include/net/dst.h:
>>   496 static inline int dst_input(struct sk_buff *skb) {
>>   498         return skb_dst(skb)->input(skb);
>>   499 }
>>
>> Is there any reason not to check to see if this pointer is NULL before
>> blindly calling it ?
>
> Sure. It can not be NULL at this point.
>

It clearly can be NULL at this point, since there are kernel panics for 
15 years of this -- going all the way back to 2.2.20 !

https://lists.gt.net/linux/kernel/250077

It seems like it might actually be a good idea to do as Alex Bligh 
suggested all those years ago and I re-suggested now.

> Just look at the code in ip_rcv_finish() :
>
> It first make sure to get a valid dst.
>
> Something is broken, probably in bridge netfilter code.
>

I'll try poking around there.

> I suspect the dst here points to &br->fake_rtable, and this is not
> expected.
>
> br_drop_fake_rtable() should have been called somewhere ...
>

Thanks,
 	Roy Keene

Re: ip_rcv_finish() NULL pointer kernel panic

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Though possibly with different things not setting the "input" function 
> > pointer in the "struct dst_entry".
> > 
> > include/net/dst.h:
> >   496 static inline int dst_input(struct sk_buff *skb) {
> >   498         return skb_dst(skb)->input(skb);
> >   499 }
> > 
> > Is there any reason not to check to see if this pointer is NULL before 
> > blindly calling it ?
> 
> Sure. It can not be NULL at this point.
> 
> Just look at the code in ip_rcv_finish() :
> 
> It first make sure to get a valid dst.
> 
> Something is broken, probably in bridge netfilter code.
> 
> I suspect the dst here points to &br->fake_rtable, and this is not
> expected.
> 
> br_drop_fake_rtable() should have been called somewhere ...

I think it makes sense to set dst->incoming
to a stub in br_netfilter_rtable_init() to just kfree_skb()+
WARN_ON_ONCE(), no need to add code to ip stack or crash kernel
due to brnf bug.

Re: ip_rcv_finish() NULL pointer kernel panic

[Eric Dumazet]

On Thu, 2017-01-26 at 17:24 +0100, Florian Westphal wrote:

> I think it makes sense to set dst->incoming
> to a stub in br_netfilter_rtable_init() to just kfree_skb()+
> WARN_ON_ONCE(), no need to add code to ip stack or crash kernel
> due to brnf bug.

Just kfree_skb() would hide bugs.

Dropping packets is not uncommon in networking...

I would rather use at least a WARN_ON_ONCE() before the kfree_skb() ;)

Re: ip_rcv_finish() NULL pointer kernel panic

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Thu, 26 Jan 2017 17:24:33 +0100

> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > Though possibly with different things not setting the "input" function 
>> > pointer in the "struct dst_entry".
>> > 
>> > include/net/dst.h:
>> >   496 static inline int dst_input(struct sk_buff *skb) {
>> >   498         return skb_dst(skb)->input(skb);
>> >   499 }
>> > 
>> > Is there any reason not to check to see if this pointer is NULL before 
>> > blindly calling it ?
>> 
>> Sure. It can not be NULL at this point.
>> 
>> Just look at the code in ip_rcv_finish() :
>> 
>> It first make sure to get a valid dst.
>> 
>> Something is broken, probably in bridge netfilter code.
>> 
>> I suspect the dst here points to &br->fake_rtable, and this is not
>> expected.
>> 
>> br_drop_fake_rtable() should have been called somewhere ...
> 
> I think it makes sense to set dst->incoming
> to a stub in br_netfilter_rtable_init() to just kfree_skb()+
> WARN_ON_ONCE(), no need to add code to ip stack or crash kernel
> due to brnf bug.

That would certainly make recovery from such bugs must better.

But I have to say that this netfilter bridging fake dst has caused
several dozen bugs over the years, it is fundamentally a serious
problem in and of itself.  It provides DST facilities by hand, in a
static object, without using any of the usual methods for creating and
facilitating dst objects.

Therefore every time someone makes an adjustment to common dst code,
this turd (and yes, it _is_ a turd) breaks.  Every single time.

So in the long term, instead of polishing this turd, let's get rid of
it.

Re: ip_rcv_finish() NULL pointer kernel panic

[Eric Dumazet]

On Thu, 2017-01-26 at 10:00 -0800, Eric Dumazet wrote:
> On Thu, 2017-01-26 at 17:24 +0100, Florian Westphal wrote:
> 
> > I think it makes sense to set dst->incoming
> > to a stub in br_netfilter_rtable_init() to just kfree_skb()+
> > WARN_ON_ONCE(), no need to add code to ip stack or crash kernel
> > due to brnf bug.
> 
> Just kfree_skb() would hide bugs.
> 
> Dropping packets is not uncommon in networking...
> 
> I would rather use at least a WARN_ON_ONCE() before the kfree_skb() ;)


Oh well, I obviously did not parse properly your suggestion.

Re: ip_rcv_finish() NULL pointer kernel panic

[Dan Streetman]

On Thu, Jan 26, 2017 at 1:04 PM, David Miller <davem@davemloft.net> wrote:
> From: Florian Westphal <fw@strlen.de>
> Date: Thu, 26 Jan 2017 17:24:33 +0100
>
>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> > Though possibly with different things not setting the "input" function
>>> > pointer in the "struct dst_entry".
>>> >
>>> > include/net/dst.h:
>>> >   496 static inline int dst_input(struct sk_buff *skb) {
>>> >   498         return skb_dst(skb)->input(skb);
>>> >   499 }
>>> >
>>> > Is there any reason not to check to see if this pointer is NULL before
>>> > blindly calling it ?
>>>
>>> Sure. It can not be NULL at this point.
>>>
>>> Just look at the code in ip_rcv_finish() :
>>>
>>> It first make sure to get a valid dst.
>>>
>>> Something is broken, probably in bridge netfilter code.
>>>
>>> I suspect the dst here points to &br->fake_rtable, and this is not
>>> expected.
>>>
>>> br_drop_fake_rtable() should have been called somewhere ...
>>
>> I think it makes sense to set dst->incoming
>> to a stub in br_netfilter_rtable_init() to just kfree_skb()+
>> WARN_ON_ONCE(), no need to add code to ip stack or crash kernel
>> due to brnf bug.
>
> That would certainly make recovery from such bugs must better.
>
> But I have to say that this netfilter bridging fake dst has caused
> several dozen bugs over the years, it is fundamentally a serious
> problem in and of itself.  It provides DST facilities by hand, in a
> static object, without using any of the usual methods for creating and
> facilitating dst objects.
>
> Therefore every time someone makes an adjustment to common dst code,
> this turd (and yes, it _is_ a turd) breaks.  Every single time.
>
> So in the long term, instead of polishing this turd, let's get rid of
> it.

I'm getting reports of this bug as well; is anyone working on removing
the bridge fake dst?

Re: ip_rcv_finish() NULL pointer kernel panic

[Florian Westphal]

Dan Streetman <dan.streetman@canonical.com> wrote:
> > But I have to say that this netfilter bridging fake dst has caused
> > several dozen bugs over the years, it is fundamentally a serious
> > problem in and of itself.  It provides DST facilities by hand, in a
> > static object, without using any of the usual methods for creating and
> > facilitating dst objects.
> >
> > Therefore every time someone makes an adjustment to common dst code,
> > this turd (and yes, it _is_ a turd) breaks.  Every single time.
> >
> > So in the long term, instead of polishing this turd, let's get rid of
> > it.
> 
> I'm getting reports of this bug as well; is anyone working on removing
> the bridge fake dst?

I don't see how we can ever remove it (unless we remove the
call-iptables feature of course).