Re: [Cake] [PATCH net v2] sched: consistently handle layer3 header accesses in the presence of VLANs

[Cong Wang <xiyou.wangcong@gmail.com>]

On Fri, Jul 3, 2020 at 8:22 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index b05e855f1ddd..d0c1cb0d264d 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -308,6 +308,35 @@ static inline bool eth_type_vlan(__be16 ethertype)
>         }
>  }
>
> +/* A getter for the SKB protocol field which will handle VLAN tags consistently
> + * whether VLAN acceleration is enabled or not.
> + */
> +static inline __be16 skb_protocol(const struct sk_buff *skb, bool skip_vlan)
> +{
> +       unsigned int offset = skb_mac_offset(skb) + sizeof(struct ethhdr);
> +       __be16 proto = skb->protocol;
> +       struct vlan_hdr vhdr, *vh;
> +
> +       if (!skip_vlan)
> +               /* VLAN acceleration strips the VLAN header from the skb and
> +                * moves it to skb->vlan_proto
> +                */
> +               return skb_vlan_tag_present(skb) ? skb->vlan_proto : proto;
> +
> +       while (eth_type_vlan(proto)) {
> +               vh = skb_header_pointer(skb, offset, sizeof(vhdr), &vhdr);
> +               if (!vh)
> +                       break;
> +
> +               proto = vh->h_vlan_encapsulated_proto;
> +               offset += sizeof(vhdr);
> +       }
> +
> +       return proto;
> +}
> +
> +
> +

Just nit: too many newlines here. Please run checkpatch.pl.


>  static inline bool vlan_hw_offload_capable(netdev_features_t features,
>                                            __be16 proto)
>  {
> diff --git a/include/net/inet_ecn.h b/include/net/inet_ecn.h
> index 0f0d1efe06dd..82763ba597f2 100644
> --- a/include/net/inet_ecn.h
> +++ b/include/net/inet_ecn.h
> @@ -4,6 +4,7 @@
>
>  #include <linux/ip.h>
>  #include <linux/skbuff.h>
> +#include <linux/if_vlan.h>
>
>  #include <net/inet_sock.h>
>  #include <net/dsfield.h>
> @@ -172,7 +173,7 @@ static inline void ipv6_copy_dscp(unsigned int dscp, struct ipv6hdr *inner)
>
>  static inline int INET_ECN_set_ce(struct sk_buff *skb)
>  {
> -       switch (skb->protocol) {
> +       switch (skb_protocol(skb, true)) {
>         case cpu_to_be16(ETH_P_IP):
>                 if (skb_network_header(skb) + sizeof(struct iphdr) <=
>                     skb_tail_pointer(skb))
> @@ -191,7 +192,7 @@ static inline int INET_ECN_set_ce(struct sk_buff *skb)
>
>  static inline int INET_ECN_set_ect1(struct sk_buff *skb)
>  {
> -       switch (skb->protocol) {
> +       switch (skb_protocol(skb, true)) {
>         case cpu_to_be16(ETH_P_IP):
>                 if (skb_network_header(skb) + sizeof(struct iphdr) <=
>                     skb_tail_pointer(skb))

These two helpers are called by non-net_sched too, are you sure
your change is correct for them too?

For example, IP6_ECN_decapsulate() uses skb->protocol then calls
INET_ECN_decapsulate() which calls the above, after your change
they use skb_protocol(). This looks inconsistent to me.

Thanks.