netdev - Re: [patch net-next-2.6] net: vlan: make non-hw-accel rx path similar to hw-accel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D989100.1090207@gmail.com>
Date:	Sun, 03 Apr 2011 17:23:44 +0200
From:	Nicolas de Pesloüan 
	<nicolas.2p.debian@...il.com>
To:	Jiri Pirko <jpirko@...hat.com>
CC:	netdev@...r.kernel.org, davem@...emloft.net,
	shemminger@...ux-foundation.org, kaber@...sh.net, fubar@...ibm.com,
	eric.dumazet@...il.com, andy@...yhouse.net, xiaosuo@...il.com,
	jesse@...ira.com
Subject: Re: [patch net-next-2.6] net: vlan: make non-hw-accel rx path similar
 to hw-accel

Le 02/04/2011 12:26, Jiri Pirko a écrit :
> Now there are 2 paths for rx vlan frames. When rx-vlan-hw-accel is
> enabled, skb is untagged by NIC, vlan_tci is set and the skb gets into
> vlan code in __netif_receive_skb - vlan_hwaccel_do_receive.
>
> For non-rx-vlan-hw-accel however, tagged skb goes thru whole
> __netif_receive_skb, it's untagged in ptype_base hander and reinjected
>
> This incosistency is fixed by this patch. Vlan untagging happens early in
> __netif_receive_skb so the rest of code (ptype_all handlers, rx_handlers)
> see the skb like it was untagged by hw.
>
> Signed-off-by: Jiri Pirko<jpirko@...hat.com>
<snip>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3da9fb0..bfe9fce 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3130,6 +3130,12 @@ another_round:
>
>   	__this_cpu_inc(softnet_data.processed);
>
> +	if (skb->protocol == cpu_to_be16(ETH_P_8021Q)) {
> +		skb = vlan_untag(skb);
> +		if (unlikely(!skb))
> +			goto out;
> +	}
> +

I like the general idea of this patch, but I don't like the idea of re-inserting specific code 
inside __netif_receive_skb.

You made a great work removing most - if not all - device specific parts from __netif_receive_skb, 
by introducing rx_handler.

I think the above part (and vlan_untag) should be moved to a vlan_rx_handler that would be set on 
the net_devices that are the parent of a vlan net_device and are NOT hwaccel.

vlan_rx_handler would return RX_HANDLER_ANOTHER if skb holds a tagged frame (skb->dev changed) and 
RX_HANDLER_PASS if skb holds an untagged frame (skb->dev unchanged).

This would also cause protocol handlers to receive the untouched (tagged) frame, if no setup 
required the frame to be untagged, which I think is the right thing to do.

 > @@ -3177,7 +3183,7 @@ ncls:
 >   			ret = deliver_skb(skb, pt_prev, orig_dev);
 >   			pt_prev = NULL;
 >   		}
 > -		if (vlan_hwaccel_do_receive(&skb)) {
 > +		if (vlan_do_receive(&skb)) {
 >   			ret = __netif_receive_skb(skb);
 >   			goto out;
 >   		} else if (unlikely(!skb))

Why are you calling __netif_receive_skb here? Can't we simply goto another_round?

I really think vlan_untag and vlan_do_receive could me merged in a vlan_rx_handler.

And if someone consider rx_handler processing happens to late for ptype_all handlers, may be it is 
time to have a look at one of my previous proposed patch: http://patchwork.ozlabs.org/patch/85578/

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