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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> 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.

Hmm, I did run checkpatch, but seems it only complains about multiple
newlines when run with --strict. Will fix, thanks! :)

>>  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.

Good point. I'll change IP{,6}_ECN_decapsulate() to also use
skb_protocol().

-Toke