netdev - Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

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]

Message-ID: <b644cecd-7954-4fa6-99e9-af8c98efe3e1@bp.renesas.com>
Date: Thu, 3 Oct 2024 11:26:07 +0100
From: Paul Barker <paul.barker.ct@...renesas.com>
To: Sergey Shtylyov <s.shtylyov@....ru>, Paul Barker <paul@...rker.dev>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>
Cc: Geert Uytterhoeven <geert+renesas@...der.be>,
 Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
 Biju Das <biju.das.jz@...renesas.com>,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, netdev@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

On 30/09/2024 21:36, Sergey Shtylyov wrote:
> On 9/30/24 19:08, Paul Barker wrote:
> 
>> From: Paul Barker <paul.barker.ct@...renesas.com>
>>
>> The GbEth IP supports offloading checksum calculation for VLAN-tagged
>> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
>> present.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@...renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 832132d44fb4..eb7499d42a9b 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>>  
>>  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>>  {
>> -	/* TODO: Need to add support for VLAN tag 802.1Q */
>> -	if (skb_vlan_tag_present(skb))
>> +	u16 net_protocol = ntohs(skb->protocol);
>> +
>> +	/* GbEth IP can calculate the checksum if:
>> +	 * - there are zero or one VLAN headers with TPID=0x8100
>> +	 * - the network protocol is IPv4 or IPv6
>> +	 * - the transport protocol is TCP, UDP or ICMP
>> +	 * - the packet is not fragmented
>> +	 */
>> +
>> +	if (skb_vlan_tag_present(skb) &&
>> +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
> 
>    Not sure I understand this check... Maybe s/==/!=/?

So, after a bit more investigation, I think this was based on a faulty
understanding. I can't find any clear documentation of this so I've gone
wandering through the code.

In vlan_dev_init() in net/8021q/vlan_dev.c, there is a check for
vlan_hw_offload_capable() on the underlying network device. If this is
false (as it will be for the GbEth IP), a set of header_ops is selected
which inserts the vlan tag into the skb data. So, we can ignore
skb->vlan_proto as skb->protocol will always be set to the VLAN protocol
for VLAN tagged packets.

The conclusion is that we can drop this if condition completely and just
keep the following if (net_protocol == ETH_P_8021Q) condition.

Will fix this in v2...

Thanks,

-- 
Paul Barker
Download attachment "OpenPGP_0x27F4B3459F002257.asc" of type "application/pgp-keys" (3521 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)