[net] bridge: clean the nf_bridge status when forwarding the skb

From: Antonio Quartulli <antonio@open-mesh.com>

Even if it is forbidden to enslave a bridge interface into
another one, it is still possible to create a chain of
virtual interfaces including two distinct bridges.

In this case, the skb entering the second bridge could have
the nf_bridge field already set due to a previous operation
and consequently lead to wrong a processing of the packet
itself.

To prevent this behaviour release and set to NULL the
nf_bridge field of the skb when forwarding the packet.

Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
---

I know that not using "extern" when declaring the prototype is not consistent
with the surrounding code, but checkpatch complaints about using "extern" in .h
file and I prefer to not do something "wrong" even if stylistically ugly.

Cheers,


 net/bridge/br_forward.c   |  2 ++
 net/bridge/br_netfilter.c | 10 ++++++++++
 net/bridge/br_private.h   |  2 ++
 3 files changed, 14 insertions(+)

Hi,

On Tue, Oct 15, 2013 at 12:51:36AM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
> 
> Even if it is forbidden to enslave a bridge interface into
> another one, it is still possible to create a chain of
> virtual interfaces including two distinct bridges.
>
> In this case, the skb entering the second bridge could have
> the nf_bridge field already set due to a previous operation
> and consequently lead to wrong a processing of the packet
> itself.
> 
> To prevent this behaviour release and set to NULL the
> nf_bridge field of the skb when forwarding the packet.
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> ---
> 
> I know that not using "extern" when declaring the prototype is not consistent
> with the surrounding code, but checkpatch complaints about using "extern" in .h
> file and I prefer to not do something "wrong" even if stylistically ugly.
> 
> Cheers,
> 
> 
>  net/bridge/br_forward.c   |  2 ++
>  net/bridge/br_netfilter.c | 10 ++++++++++
>  net/bridge/br_private.h   |  2 ++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index 4b81b14..62955f3 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -49,6 +49,8 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)
>  	} else {
>  		skb_push(skb, ETH_HLEN);
>  		br_drop_fake_rtable(skb);
> +		br_netfilter_skb_free(skb);
> +
>  		dev_queue_xmit(skb);
>  	}
>  
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index f877362..7cad3e2 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -1086,3 +1086,13 @@ void br_netfilter_fini(void)
>  #endif
>  	dst_entries_destroy(&fake_dst_ops);
>  }
> +
> +/**
> + * br_netfilter_skb_free - clean the NF bridge data in an skb
> + * @skb: the skb which the data to free belongs to
> + */
> +void br_netfilter_skb_free(struct sk_buff *skb)
> +{
> +	nf_bridge_put(skb->nf_bridge);
> +	skb->nf_bridge = NULL;
> +}

This should be nf_reset.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:
> Hi,
> > +
> > +/**
> > + * br_netfilter_skb_free - clean the NF bridge data in an skb
> > + * @skb: the skb which the data to free belongs to
> > + */
> > +void br_netfilter_skb_free(struct sk_buff *skb)
> > +{
> > +	nf_bridge_put(skb->nf_bridge);
> > +	skb->nf_bridge = NULL;
> > +}
> 
> This should be nf_reset.

You think I should directly use nf_reset instead of this function?

I see that nf_reset() cleans up the conntrack part too: does it also become
useless once the packet exits the bridge interface?


Cheers,

On Thu, Oct 17, 2013 at 01:37:35PM +0200, Antonio Quartulli wrote:
> On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:
> > Hi,
> > > +
> > > +/**
> > > + * br_netfilter_skb_free - clean the NF bridge data in an skb
> > > + * @skb: the skb which the data to free belongs to
> > > + */
> > > +void br_netfilter_skb_free(struct sk_buff *skb)
> > > +{
> > > +	nf_bridge_put(skb->nf_bridge);
> > > +	skb->nf_bridge = NULL;
> > > +}
> > 
> > This should be nf_reset.
> 
> You think I should directly use nf_reset instead of this function?
> 
> I see that nf_reset() cleans up the conntrack part too: does it also become
> useless once the packet exits the bridge interface?

The conntrack should not attached if it's forwarded to another netif,
see dev_forward_skb.

But I'm not sure what scenario you're trying to handle with this
change, if you could please elaborate.

Perhaps your fix is more conservative to avoid breaking strange setups
that have been relying on this behaviour. I know of people deploying
strange configurations using netfilter bridge.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 18, 2013 at 01:10:41PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2013 at 01:37:35PM +0200, Antonio Quartulli wrote:
> > On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:
> > > Hi,
> > > > +
> > > > +/**
> > > > + * br_netfilter_skb_free - clean the NF bridge data in an skb
> > > > + * @skb: the skb which the data to free belongs to
> > > > + */
> > > > +void br_netfilter_skb_free(struct sk_buff *skb)
> > > > +{
> > > > +	nf_bridge_put(skb->nf_bridge);
> > > > +	skb->nf_bridge = NULL;
> > > > +}
> > > 
> > > This should be nf_reset.
> > 
> > You think I should directly use nf_reset instead of this function?
> > 
> > I see that nf_reset() cleans up the conntrack part too: does it also become
> > useless once the packet exits the bridge interface?
> 
> The conntrack should not attached if it's forwarded to another netif,
> see dev_forward_skb.
> 
> But I'm not sure what scenario you're trying to handle with this
> change, if you could please elaborate.


This is a sample scenario (nf bridge is on):

[eth0] ---> [br0] ---> [bat0] ---> [br1]

where the relation '[a] ---> [b]' means 'a is enslaved in b' (bat0 is a
batman-adv virtual interface..in this situation it should not matter: it
just removs an header from an incoming skb and delivers it).

The problem I was having was due to an skb entering br0 first and br1 later.
When reaching br1 skb->nf_bridge was != NULL because of the previous processing
in br0.

To clarify, the packet arriving on eth0 is 'delivered' to br0. It is not
forwarded to another port of the bridge. Therefore I am not sure that we should
clean the conntrack part too.

> 
> Perhaps your fix is more conservative to avoid breaking strange setups
> that have been relying on this behaviour. I know of people deploying
> strange configurations using netfilter bridge.
> 

could be.

Cheers,

On 10/18/2013 07:35 AM, Antonio Quartulli wrote:
> On Fri, Oct 18, 2013 at 01:10:41PM +0200, Pablo Neira Ayuso wrote:
>> On Thu, Oct 17, 2013 at 01:37:35PM +0200, Antonio Quartulli wrote:
>>> On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:
>>>> Hi,
>>>>> +
>>>>> +/**
>>>>> + * br_netfilter_skb_free - clean the NF bridge data in an skb
>>>>> + * @skb: the skb which the data to free belongs to
>>>>> + */
>>>>> +void br_netfilter_skb_free(struct sk_buff *skb)
>>>>> +{
>>>>> +	nf_bridge_put(skb->nf_bridge);
>>>>> +	skb->nf_bridge = NULL;
>>>>> +}
>>>>
>>>> This should be nf_reset.
>>>
>>> You think I should directly use nf_reset instead of this function?
>>>
>>> I see that nf_reset() cleans up the conntrack part too: does it also become
>>> useless once the packet exits the bridge interface?
>>
>> The conntrack should not attached if it's forwarded to another netif,
>> see dev_forward_skb.
>>
>> But I'm not sure what scenario you're trying to handle with this
>> change, if you could please elaborate.
>
>
> This is a sample scenario (nf bridge is on):
>
> [eth0] ---> [br0] ---> [bat0] ---> [br1]
>

Another possible config that is out in the wild is

[eth0] ---> [br0] ---> [vlanX] ----> [br1]


> where the relation '[a] ---> [b]' means 'a is enslaved in b' (bat0 is a
> batman-adv virtual interface..in this situation it should not matter: it
> just removs an header from an incoming skb and delivers it).
>
> The problem I was having was due to an skb entering br0 first and br1 later.
> When reaching br1 skb->nf_bridge was != NULL because of the previous processing
> in br0.
>

Doesn't br_nf_pre_routing already take care of this for you?  It will 
drop the ref on the current nf_bridge and allocate a new one.  Is that
not sufficient?

-vlad

> To clarify, the packet arriving on eth0 is 'delivered' to br0. It is not
> forwarded to another port of the bridge. Therefore I am not sure that we should
> clean the conntrack part too.
>
>>
>> Perhaps your fix is more conservative to avoid breaking strange setups
>> that have been relying on this behaviour. I know of people deploying
>> strange configurations using netfilter bridge.
>>
>
> could be.
>
> Cheers,
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 18, 2013 at 10:32:09AM -0400, Vlad Yasevich wrote:
> On 10/18/2013 07:35 AM, Antonio Quartulli wrote:
> > On Fri, Oct 18, 2013 at 01:10:41PM +0200, Pablo Neira Ayuso wrote:
> >> On Thu, Oct 17, 2013 at 01:37:35PM +0200, Antonio Quartulli wrote:
> >>> On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:

[...]

> >
> > The problem I was having was due to an skb entering br0 first and br1 later.
> > When reaching br1 skb->nf_bridge was != NULL because of the previous processing
> > in br0.
> >
> 
> Doesn't br_nf_pre_routing already take care of this for you?  It will 
> drop the ref on the current nf_bridge and allocate a new one.  Is that
> not sufficient?

In my case that line is not reached because

 700         if (!IS_IP(skb) && !IS_VLAN_IP(skb) && !IS_PPPOE_IP(skb))

is always true: the packet getting analysed is a batman-adv encapsulated packet,
which does not match any of the three above.

Cheers,

On 10/18/2013 10:46 AM, Antonio Quartulli wrote:
> On Fri, Oct 18, 2013 at 10:32:09AM -0400, Vlad Yasevich wrote:
>> On 10/18/2013 07:35 AM, Antonio Quartulli wrote:
>>> On Fri, Oct 18, 2013 at 01:10:41PM +0200, Pablo Neira Ayuso wrote:
>>>> On Thu, Oct 17, 2013 at 01:37:35PM +0200, Antonio Quartulli wrote:
>>>>> On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:
>
> [...]
>
>>>
>>> The problem I was having was due to an skb entering br0 first and br1 later.
>>> When reaching br1 skb->nf_bridge was != NULL because of the previous processing
>>> in br0.
>>>
>>
>> Doesn't br_nf_pre_routing already take care of this for you?  It will
>> drop the ref on the current nf_bridge and allocate a new one.  Is that
>> not sufficient?
>
> In my case that line is not reached because
>
>   700         if (!IS_IP(skb) && !IS_VLAN_IP(skb) && !IS_PPPOE_IP(skb))
>
> is always true: the packet getting analysed is a batman-adv encapsulated packet,
> which does not match any of the three above.
>
> Cheers,
>

Looking at other encapsulators (PPP, iptunnel, VXLAN), they do
nf_reset() on input.  Would that be appropriate for batman as well?

-vlad
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 18, 2013 at 11:33:06AM -0400, Vlad Yasevich wrote:
> On 10/18/2013 10:46 AM, Antonio Quartulli wrote:
> > On Fri, Oct 18, 2013 at 10:32:09AM -0400, Vlad Yasevich wrote:
> >> On 10/18/2013 07:35 AM, Antonio Quartulli wrote:
> >>> On Fri, Oct 18, 2013 at 01:10:41PM +0200, Pablo Neira Ayuso wrote:
> >>>> On Thu, Oct 17, 2013 at 01:37:35PM +0200, Antonio Quartulli wrote:
> >>>>> On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:
> >
> > [...]
> >
> >>>
> >>> The problem I was having was due to an skb entering br0 first and br1 later.
> >>> When reaching br1 skb->nf_bridge was != NULL because of the previous processing
> >>> in br0.
> >>>
> >>
> >> Doesn't br_nf_pre_routing already take care of this for you?  It will
> >> drop the ref on the current nf_bridge and allocate a new one.  Is that
> >> not sufficient?
> >
> > In my case that line is not reached because
> >
> >   700         if (!IS_IP(skb) && !IS_VLAN_IP(skb) && !IS_PPPOE_IP(skb))
> >
> > is always true: the packet getting analysed is a batman-adv encapsulated packet,
> > which does not match any of the three above.
> >
> > Cheers,
> >
> 
> Looking at other encapsulators (PPP, iptunnel, VXLAN), they do
> nf_reset() on input.  Would that be appropriate for batman as well?

I thought that too.

But at this point, wouldn't it be better to do a reset here and remove the other
resets from any other encapsulation module?

Maybe this operation is supposed to not happen if no encapsulation is involved?
I thought that polishing the nf state when exiting the nf related path was a
clean and easy solution.

Moreover we avoid that any newly implemented tunneling module hit this problem again.


Cheers,

On 10/18/2013 11:41 AM, Antonio Quartulli wrote:
> On Fri, Oct 18, 2013 at 11:33:06AM -0400, Vlad Yasevich wrote:
>> On 10/18/2013 10:46 AM, Antonio Quartulli wrote:
>>> On Fri, Oct 18, 2013 at 10:32:09AM -0400, Vlad Yasevich wrote:
>>>> On 10/18/2013 07:35 AM, Antonio Quartulli wrote:
>>>>> On Fri, Oct 18, 2013 at 01:10:41PM +0200, Pablo Neira Ayuso wrote:
>>>>>> On Thu, Oct 17, 2013 at 01:37:35PM +0200, Antonio Quartulli wrote:
>>>>>>> On Thu, Oct 17, 2013 at 04:28:57AM -0700, Pablo Neira Ayuso wrote:
>>>
>>> [...]
>>>
>>>>>
>>>>> The problem I was having was due to an skb entering br0 first and br1 later.
>>>>> When reaching br1 skb->nf_bridge was != NULL because of the previous processing
>>>>> in br0.
>>>>>
>>>>
>>>> Doesn't br_nf_pre_routing already take care of this for you?  It will
>>>> drop the ref on the current nf_bridge and allocate a new one.  Is that
>>>> not sufficient?
>>>
>>> In my case that line is not reached because
>>>
>>>    700         if (!IS_IP(skb) && !IS_VLAN_IP(skb) && !IS_PPPOE_IP(skb))
>>>
>>> is always true: the packet getting analysed is a batman-adv encapsulated packet,
>>> which does not match any of the three above.
>>>
>>> Cheers,
>>>
>>
>> Looking at other encapsulators (PPP, iptunnel, VXLAN), they do
>> nf_reset() on input.  Would that be appropriate for batman as well?
>
> I thought that too.
>
> But at this point, wouldn't it be better to do a reset here and remove the other
> resets from any other encapsulation module?
>
> Maybe this operation is supposed to not happen if no encapsulation is involved?

This is exactly right.  The reset happens much later if there is no 
encapsulation.  However, if there is an encapsuation that changes the 
hader values that are used to filter, then nf_reset has to happen.
That is why nf_reset happens input to the encapsulation layer instead
of always on output from bridge.

-vlad

> I thought that polishing the nf state when exiting the nf related path was a
> clean and easy solution.
>
> Moreover we avoid that any newly implemented tunneling module hit this problem again.
>
>
> Cheers,
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Oct 21, 2013 at 11:00:48AM -0400, Vlad Yasevich wrote:
> >>
> >> Looking at other encapsulators (PPP, iptunnel, VXLAN), they do
> >> nf_reset() on input.  Would that be appropriate for batman as well?
> >
> > I thought that too.
> >
> > But at this point, wouldn't it be better to do a reset here and remove the other
> > resets from any other encapsulation module?
> >
> > Maybe this operation is supposed to not happen if no encapsulation is involved?
> 
> This is exactly right.  The reset happens much later if there is no 
> encapsulation.  However, if there is an encapsuation that changes the 
> hader values that are used to filter, then nf_reset has to happen.
> That is why nf_reset happens input to the encapsulation layer instead
> of always on output from bridge.

I see.

Then I think that this patch can be dropped.
I will provide another patch to batman-adv so that it can reset the nf state
before extracting the payload packet.


Thanks a lot for your feedback!

Regards,