Re: [PATCH net] stmmac: fix reception of 802.1ad Ethernet tagged frames

[Elad Nachman <eladv6@gmail.com>]

Currently running:
ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100

On eth0=stmmac succeeds, but the end result is that the vlan device gets proto 802.1q instead of proto 802.1ad and drops the received packet. Without the patch packets gets dropped for a seemingly "correct" 802.1ad ip link configuration.

If NETIF_F_HW_VLAN_STAG_RX is a requirement for the driver for supporting 802.1ad protocols then the Linux kernel should return error when user-space requests to create a vlan device with proto 802.1ad for physical devices which lacks NETIF_F_HW_VLAN_STAG_RX, which is not currently the case.

skb_vlan_untag() does nothing if __vlan_hwaccel_put_tag() was already called before (in the driver). The only possible alternative is to completely remove stmmac_rx_vlan() from the stmmac code and let skb_vlan_untag() handles things in a generic way.


On 08/05/18 09:43, Toshiaki Makita wrote:
> On 2018/05/08 15:01, Elad Nachman wrote:
>> stmmac reception handler calls stmmac_rx_vlan() to strip the vlan before calling napi_gro_receive().
>>
>> The function assumes VLAN tagged frames are always tagged with 802.1Q protocol,
>> and assigns ETH_P_8021Q to the skb by hard-coding the parameter on call to __vlan_hwaccel_put_tag() .
>>
>> This causes packets not to be passed to the VLAN slave if it was created with 802.1AD protocol
>> (ip link add link eth0 eth0.100 type vlan proto 802.1ad id 100).
>>
>> This fix passes the protocol from the VLAN header into __vlan_hwaccel_put_tag()
>> instead of using the hard-coded value of ETH_P_8021Q.
>>
>> Signed-off-by: Elad Nachman <eladn@gilat.com>
>>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index b65e2d1..ced2d34 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -3293,17 +3293,19 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>  
>>  static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
>>  {
>> -	struct ethhdr *ehdr;
>> +	struct vlan_ethhdr *veth;
>>  	u16 vlanid;
>> +	__be16 vlan_proto;
>>  
>>  	if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) ==
>>  	    NETIF_F_HW_VLAN_CTAG_RX &&
>>  	    !__vlan_get_tag(skb, &vlanid)) {
>>  		/* pop the vlan tag */
>> -		ehdr = (struct ethhdr *)skb->data;
>> -		memmove(skb->data + VLAN_HLEN, ehdr, ETH_ALEN * 2);
>> +		veth = (struct vlan_ethhdr *)skb->data;
>> +		vlan_proto = veth->h_vlan_proto;
>> +		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
>>  		skb_pull(skb, VLAN_HLEN);
>> -		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlanid);
>> +		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
> 
> This is what devices with NETIF_F_HW_VLAN_STAG_RX are supposed to do,
> not NETIF_F_HW_VLAN_CTAG_RX.
> 
> By the way this looks like doing the same thing as skb_vlan_untag in
> __netif_receive_skb_core, so seems unnecessary to add HW_VLAN_STAG_RX.
> Alternatively you can check if vlan_proto is 8021Q here.
>