Re: [Linux-ATM-General] [RFC][PATCH 0/2] atm: br2682: Improve encapsulation checks and headroom allocation

[chas w. - C. <ch...@cm...>]

On Fri, 12 Aug 2011 22:01:10 +0200
Pascal Hambourg <pa...@pl...> wrote:

> chas williams - CONTRACTOR a écrit :
> > On Mon, 08 Aug 2011 00:05:24 +0200
> > Pascal Hambourg <pa...@pl...> wrote:
> > 
> >> Also it appears that eth_type_trans(), which is called in the bridged receive
> >> path in br2684_push(), calls skb_pull(skb, ETH_HLEN). Shouldn't the packet
> >> length be checked before calling eth_type_trans() ?
> > 
> > probably.  i dont see any reason it shouldnt be checked.
> > 
> >> Similarly, shouldn't the packet length be checked in the VC-MUX routed path
> >> before reading the IP header version ?
> > 
> > yes, that seems reasonable.
> 
> I updated and resent the patch 1/2 (v2) accordingly :
> 
> - Check that the LLC header matches the expected payload type.
> - Check that the PAD field is 0x00-00 also in LLC bridged mode.
> - Check that the frame length in bridged mode is at least ETH_HLEN (14)
>   without FCS or ETH_ZLEN + ETH_FCS_LEN (64) with FCS.
> - Trim the trailing FCS field only if the PID is ethernet with FCS.
> - Check the data length is not null before reading the IP header version
>   field in VC-MUX routed mode.

this looks fine at a glance.  i do wonder if:

*((u16 *) (skb->data + 8)) == 0

works on all platforms.  cpu's other than x86 sometimes have issues
with arbitrary alignments of data.   it should probalby be a memcmp()
but that should probably be fixed in a seperate patch.

> The optimizations you suggested are way beyond my skill and knowledge of the
> kernel code. My goal here was just to provide fixes with minimal changes which
> could also be easily backported to the -stable and -longterm trees.

my only suggestion is that besides getting minheadroom correct, is
that you should set hard_headerlen correctly as well when the interface
is setup.  in br2684_setup_routed() you should also do something
like netdev->hard_header_len = 2;

sometimes this should be 0 but you dont know the encapsulation until
the vcc is registered so at this point you are best just picking the
worst case.

also, for the bridged case we know that the hard_header_len is going to 
atmost ETH_HLEN + 10 since we will also always prepend something.

this should keep the need to skb_realloc_headroom() to a minimum.