netdev - Re: [PATCH bpf-next v2 17/20] veth: Implement VLAN tag and checksum level XDP hint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 6 Jul 2023 11:57:06 +0200
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: Stanislav Fomichev <sdf@...gle.com>,
 Larysa Zaremba <larysa.zaremba@...el.com>
Cc: brouer@...hat.com, bpf@...r.kernel.org, ast@...nel.org,
 daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev,
 song@...nel.org, yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org,
 haoluo@...gle.com, jolsa@...nel.org, David Ahern <dsahern@...il.com>,
 Jakub Kicinski <kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>,
 Anatoly Burakov <anatoly.burakov@...el.com>,
 Alexander Lobakin <alexandr.lobakin@...el.com>,
 Magnus Karlsson <magnus.karlsson@...il.com>,
 Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 17/20] veth: Implement VLAN tag and checksum
 level XDP hint


On 05/07/2023 19.25, Stanislav Fomichev wrote:
> On 07/03, Larysa Zaremba wrote:
>> In order to test VLAN tag and checksum level XDP hints in
>> hardware-independent selfttests, implement newly added XDP hints in veth
>> driver.
>>
>> Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
>> ---
>>   drivers/net/veth.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> index 614f3e3efab0..a7f2b679551d 100644
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
>> @@ -1732,6 +1732,44 @@ static int veth_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash,
>>   	return 0;
>>   }
>>   
>> +static int veth_xdp_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tag,
>> +				__be16 *vlan_proto)
>> +{
>> +	struct veth_xdp_buff *_ctx = (void *)ctx;
>> +	struct sk_buff *skb = _ctx->skb;
>> +	int err;
>> +
>> +	if (!skb)
>> +		return -ENODATA;
>> +
> 
> [..]
> 
>> +	err = __vlan_hwaccel_get_tag(skb, vlan_tag);
> 
> We probably need to open code __vlan_hwaccel_get_tag here. Because it
> returns -EINVAL on !skb_vlan_tag_present where the expectation, for us,
> I'm assuming is -ENODATA?
> 

Looking at in-tree users of __vlan_hwaccel_get_tag(), they don't use the
err value for anything.  Thus, we can just change
__vlan_hwaccel_get_tag() to return -ENODATA instead of -EINVAL.  (And
also remember __vlan_get_tag() adjustmment).


$ git diff
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 6ba71957851e..fb35d7dd77a2 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -540,7 +540,7 @@ static inline int __vlan_get_tag(const struct 
sk_buff *skb, u16 *vlan_tci)
         struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);

         if (!eth_type_vlan(veth->h_vlan_proto))
-               return -EINVAL;
+               return -ENODATA;

         *vlan_tci = ntohs(veth->h_vlan_TCI);
         return 0;
@@ -561,7 +561,7 @@ static inline int __vlan_hwaccel_get_tag(const 
struct sk_buff *skb,
                 return 0;
         } else {
                 *vlan_tci = 0;
-               return -EINVAL;
+               return -ENODATA;
         }
  }



>> +	if (err)
>> +		return err;
>> +
>> +	*vlan_proto = skb->vlan_proto;
>> +	return err;
>> +}
>> +
>> +static int veth_xdp_rx_csum_lvl(const struct xdp_md *ctx, u8 *csum_level)
>> +{
>> +	struct veth_xdp_buff *_ctx = (void *)ctx;
>> +	struct sk_buff *skb = _ctx->skb;
>> +
>> +	if (!skb)
>> +		return -ENODATA;
>> +
>> +	if (skb->ip_summed == CHECKSUM_UNNECESSARY)
>> +		*csum_level = skb->csum_level;
>> +	else if (skb->ip_summed == CHECKSUM_PARTIAL &&
>> +		 skb_checksum_start_offset(skb) == skb_transport_offset(skb) ||
>> +		 skb->csum_valid)
>> +		*csum_level = 0;
>> +	else
>> +		return -ENODATA;
>> +
>> +	return 0;
>> +}
>> +
[...]