On 2018/06/03 23:33, David Miller wrote:
> From: Elad Nachman <ela...@gmail.com>
> Date: Wed, 30 May 2018 08:48:25 +0300
>
>> 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;
>
> Please order local variables from longest to shortest line.
>
>>
>> - if ((dev->features & NETIF_F_HW_VLAN_CTAG_RX) ==
>> - NETIF_F_HW_VLAN_CTAG_RX &&
>> - !__vlan_get_tag(skb, &vlanid)) {
>> + if (!__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);
>> }
>> }
>
> I can't see how it is valid to do an unconditional software VLAN
> untagging even when VLAN is disabled in the kernel config or the
> NETIF_F_* feature bits are not set.
Right. It is not valid.
>
> At a minimum that feature test has to stay there, and when it's clear
> we let the generic VLAN code untag the packet.
Since NETIF_F_HW_VLAN_*_RX are not protocol agnostic, we need two kind
of similar checking here.
veth = (struct vlan_ethhdr *)skb->data;
vlan_proto = veth->h_vlan_proto;
if ((vlan_proto == htons(ETH_P_8021Q) &&
dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
(vlan_proto == htons(ETH_P_8021AD) &&
dev->features & NETIF_F_HW_VLAN_STAG_RX) {
vlanid = ntohs(veth->h_vlan_TCI);
memmove(...);
skb_pull(...);
__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
}
An alternative way is not to check vlan_proto or features here but
compile this code only when VLAN is enabled in the kernel config. This
can be valid only because this driver does not have NETIF_F_HW_VLAN_*_RX
in hw_features and they can not be toggled for now.
static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
{
#ifdef STMMAC_VLAN_TAG_USED
...
if (!__vlan_get_tag(skb, &vlanid)) {
...
__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
}
#endif
}
--
Toshiaki Makita
