Cong Wang <xiyou.wangc...@gmail.com> writes:
> On Fri, Jul 3, 2020 at 8:22 AM Toke Høiland-Jørgensen <t...@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
