[PATCH net] r8169: fix accessing unset transport header

[PATCH net] r8169: fix accessing unset transport header

[Heiner Kallweit]

66e4c8d95008 ("net: warn if transport header was not set") added
a check that triggers a warning in r8169, see [0].

[0] https://bugzilla.kernel.org/show_bug.cgi?id=216157

Fixes: 8d520b4de3ed ("r8169: work around RTL8125 UDP hw bug")
Reported-by: Erhard F. <erhard_f@mailbox.org>
Tested-by: Erhard F. <erhard_f@mailbox.org>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 3098d6672..1b7fdb4f0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4190,7 +4190,6 @@ static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)
 static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 				struct sk_buff *skb, u32 *opts)
 {
-	u32 transport_offset = (u32)skb_transport_offset(skb);
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
 	u32 mss = shinfo->gso_size;
 
@@ -4207,7 +4206,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 			WARN_ON_ONCE(1);
 		}
 
-		opts[0] |= transport_offset << GTTCPHO_SHIFT;
+		opts[0] |= skb_transport_offset(skb) << GTTCPHO_SHIFT;
 		opts[1] |= mss << TD1_MSS_SHIFT;
 	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
 		u8 ip_protocol;
@@ -4235,7 +4234,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 		else
 			WARN_ON_ONCE(1);
 
-		opts[1] |= transport_offset << TCPHO_SHIFT;
+		opts[1] |= skb_transport_offset(skb) << TCPHO_SHIFT;
 	} else {
 		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
 
@@ -4402,14 +4401,13 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
 						struct net_device *dev,
 						netdev_features_t features)
 {
-	int transport_offset = skb_transport_offset(skb);
 	struct rtl8169_private *tp = netdev_priv(dev);
 
 	if (skb_is_gso(skb)) {
 		if (tp->mac_version == RTL_GIGA_MAC_VER_34)
 			features = rtl8168evl_fix_tso(skb, features);
 
-		if (transport_offset > GTTCPHO_MAX &&
+		if (skb_transport_offset(skb) > GTTCPHO_MAX &&
 		    rtl_chip_supports_csum_v2(tp))
 			features &= ~NETIF_F_ALL_TSO;
 	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
@@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
 		if (rtl_quirk_packet_padto(tp, skb))
 			features &= ~NETIF_F_CSUM_MASK;
 
-		if (transport_offset > TCPHO_MAX &&
+		if (skb_transport_offset(skb) > TCPHO_MAX &&
 		    rtl_chip_supports_csum_v2(tp))
 			features &= ~NETIF_F_CSUM_MASK;
 	}
-- 
2.36.1

Re: [PATCH net] r8169: fix accessing unset transport header

[Francois Romieu]

Heiner Kallweit <hkallweit1@gmail.com> :
> 66e4c8d95008 ("net: warn if transport header was not set") added
> a check that triggers a warning in r8169, see [0].
> 
> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216157
> 
> Fixes: 8d520b4de3ed ("r8169: work around RTL8125 UDP hw bug")
> Reported-by: Erhard F. <erhard_f@mailbox.org>
> Tested-by: Erhard F. <erhard_f@mailbox.org>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

/me wonders...

- bz216157 experiences a (2nd) warning because the rtl8169_tso_csum_v2
  ARP path shares the factored read of the (unset) transport offset
  but said ARP path does not use the transport offset.
  -> ok, the warning is mostly harmless. 

- rtl8169_tso_csum_v2 non-ARP paths own WARN_ON_ONCE will always
  complain before Eric's transport specific warning triggers.
  -> ok, the warning is redundant.

- rtl8169_features_check

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 3098d6672..1b7fdb4f0 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
[...]
> @@ -4402,14 +4401,13 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>  						struct net_device *dev,
>  						netdev_features_t features)
>  {
> -	int transport_offset = skb_transport_offset(skb);
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  
>  	if (skb_is_gso(skb)) {
>  		if (tp->mac_version == RTL_GIGA_MAC_VER_34)
>  			features = rtl8168evl_fix_tso(skb, features);
>  
> -		if (transport_offset > GTTCPHO_MAX &&
> +		if (skb_transport_offset(skb) > GTTCPHO_MAX &&
>  		    rtl_chip_supports_csum_v2(tp))
>  			features &= ~NETIF_F_ALL_TSO;
>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
> @@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>  		if (rtl_quirk_packet_padto(tp, skb))
>  			features &= ~NETIF_F_CSUM_MASK;
>  
> -		if (transport_offset > TCPHO_MAX &&
> +		if (skb_transport_offset(skb) > TCPHO_MAX &&
>  		    rtl_chip_supports_csum_v2(tp))
>  			features &= ~NETIF_F_CSUM_MASK;
>  	}

Neither skb_is_gso nor CHECKSUM_PARTIAL implies a transport header so the
warning may still trigger, right ?

Btw it's a bit unexpected to see a "Fixes" tag related to a RTL8125 bug as
well as a "Tested-by" by the bugzilla submitter when the dmesg included in
bz216157 exibits a RTL8168e/8111e.

-- 
Ueimor

Re: [PATCH net] r8169: fix accessing unset transport header

[Heiner Kallweit]

On 04.07.2022 02:55, Francois Romieu wrote:
> Heiner Kallweit <hkallweit1@gmail.com> :
>> 66e4c8d95008 ("net: warn if transport header was not set") added
>> a check that triggers a warning in r8169, see [0].
>>
>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216157
>>
>> Fixes: 8d520b4de3ed ("r8169: work around RTL8125 UDP hw bug")
>> Reported-by: Erhard F. <erhard_f@mailbox.org>
>> Tested-by: Erhard F. <erhard_f@mailbox.org>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> /me wonders...
> 
> - bz216157 experiences a (2nd) warning because the rtl8169_tso_csum_v2
>   ARP path shares the factored read of the (unset) transport offset
>   but said ARP path does not use the transport offset.
>   -> ok, the warning is mostly harmless. 
> 
> - rtl8169_tso_csum_v2 non-ARP paths own WARN_ON_ONCE will always
>   complain before Eric's transport specific warning triggers.
>   -> ok, the warning is redundant.
> 
> - rtl8169_features_check
> 
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index 3098d6672..1b7fdb4f0 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> [...]
>> @@ -4402,14 +4401,13 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>>  						struct net_device *dev,
>>  						netdev_features_t features)
>>  {
>> -	int transport_offset = skb_transport_offset(skb);
>>  	struct rtl8169_private *tp = netdev_priv(dev);
>>  
>>  	if (skb_is_gso(skb)) {
>>  		if (tp->mac_version == RTL_GIGA_MAC_VER_34)
>>  			features = rtl8168evl_fix_tso(skb, features);
>>  
>> -		if (transport_offset > GTTCPHO_MAX &&
>> +		if (skb_transport_offset(skb) > GTTCPHO_MAX &&
>>  		    rtl_chip_supports_csum_v2(tp))
>>  			features &= ~NETIF_F_ALL_TSO;
>>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> @@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>>  		if (rtl_quirk_packet_padto(tp, skb))
>>  			features &= ~NETIF_F_CSUM_MASK;
>>  
>> -		if (transport_offset > TCPHO_MAX &&
>> +		if (skb_transport_offset(skb) > TCPHO_MAX &&
>>  		    rtl_chip_supports_csum_v2(tp))
>>  			features &= ~NETIF_F_CSUM_MASK;
>>  	}
> 
> Neither skb_is_gso nor CHECKSUM_PARTIAL implies a transport header so the
> warning may still trigger, right ?
>

I'm not an expert here, and due to missing chip documentation I can't say
whether the chip could handle hw csumming correctly w/o transport header.
I'd see whether we get more reports of this warning. If yes, then maybe
we should use skb_transport_header_was_set() explicitly and disable
hw csumming if there's no transport header.

> Btw it's a bit unexpected to see a "Fixes" tag related to a RTL8125 bug as
> well as a "Tested-by" by the bugzilla submitter when the dmesg included in
> bz216157 exibits a RTL8168e/8111e.
> 
The Fixes tag refers to the latest change to the affected code, therefore
it comes a little unexpected, right.

Re: [PATCH net] r8169: fix accessing unset transport header

[Francois Romieu]

Heiner Kallweit <hkallweit1@gmail.com> :
> On 04.07.2022 02:55, Francois Romieu wrote:
> > Heiner Kallweit <hkallweit1@gmail.com> :
> >> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > [...]
> >> @@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
> >>  		if (rtl_quirk_packet_padto(tp, skb))
> >>  			features &= ~NETIF_F_CSUM_MASK;
> >>  
> >> -		if (transport_offset > TCPHO_MAX &&
> >> +		if (skb_transport_offset(skb) > TCPHO_MAX &&
> >>  		    rtl_chip_supports_csum_v2(tp))
> >>  			features &= ~NETIF_F_CSUM_MASK;
> >>  	}
> > 
> > Neither skb_is_gso nor CHECKSUM_PARTIAL implies a transport header so the
> > warning may still trigger, right ?
> 
> I'm not an expert here, and due to missing chip documentation I can't say
> whether the chip could handle hw csumming correctly w/o transport header.
> I'd see whether we get more reports of this warning. If yes, then maybe
> we should use skb_transport_header_was_set() explicitly and disable
> hw csumming if there's no transport header.

(some sleep later)

I had forgotten the NETIF_F_* stuff in the r8169 driver. :o/

So, yes, ignore this point.

> > Btw it's a bit unexpected to see a "Fixes" tag related to a RTL8125 bug as
> > well as a "Tested-by" by the bugzilla submitter when the dmesg included in
> > bz216157 exibits a RTL8168e/8111e.
> > 
> The Fixes tag refers to the latest change to the affected code, therefore
> it comes a little unexpected, right.

?

8d520b4de3ed does not change the affected code.

Eric's unset transport offset detection debug code would have produced the
same output with the parent of the "Fixes" commit id:

$ git cat-file -p 8d520b4de3ed^:drivers/net/ethernet/realtek/r8169_main.c | grep -A4 -B1 -E 'rtl8169_features_check'

static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
						struct net_device *dev,
						netdev_features_t features)
{
	int transport_offset = skb_transport_offset(skb);
--
	.ndo_start_xmit		= rtl8169_start_xmit,
	.ndo_features_check	= rtl8169_features_check,
	.ndo_tx_timeout		= rtl8169_tx_timeout,
	.ndo_validate_addr	= eth_validate_addr,
	.ndo_change_mtu		= rtl8169_change_mtu,
	.ndo_fix_features	= rtl8169_fix_features,


-> 8d520b4de3ed does not modify the first
'int transport_offset = skb_transport_offset(skb);' statement and neither
does it modify the code path to rtl8169_features_check 

8d520b4de3ed actually removes some logical path towards rtl8169_tso_csum_v2
but it does not change (nor does it break) the relevant code:

$ git cat-file -p 8d520b4de3ed^:drivers/net/ethernet/realtek/r8169_main.c | grep -A3 -B1 -E 'bool rtl8169_tso_csum_v2'

static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
				struct sk_buff *skb, u32 *opts)
{
	u32 transport_offset = (u32)skb_transport_offset(skb);


-- 
Ueimor

Re: [PATCH net] r8169: fix accessing unset transport header

[Heiner Kallweit]

On 04.07.2022 17:40, Francois Romieu wrote:
> Heiner Kallweit <hkallweit1@gmail.com> :
>> On 04.07.2022 02:55, Francois Romieu wrote:
>>> Heiner Kallweit <hkallweit1@gmail.com> :
>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> [...]
>>>> @@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>>>>  		if (rtl_quirk_packet_padto(tp, skb))
>>>>  			features &= ~NETIF_F_CSUM_MASK;
>>>>  
>>>> -		if (transport_offset > TCPHO_MAX &&
>>>> +		if (skb_transport_offset(skb) > TCPHO_MAX &&
>>>>  		    rtl_chip_supports_csum_v2(tp))
>>>>  			features &= ~NETIF_F_CSUM_MASK;
>>>>  	}
>>>
>>> Neither skb_is_gso nor CHECKSUM_PARTIAL implies a transport header so the
>>> warning may still trigger, right ?
>>
>> I'm not an expert here, and due to missing chip documentation I can't say
>> whether the chip could handle hw csumming correctly w/o transport header.
>> I'd see whether we get more reports of this warning. If yes, then maybe
>> we should use skb_transport_header_was_set() explicitly and disable
>> hw csumming if there's no transport header.
> 
> (some sleep later)
> 
> I had forgotten the NETIF_F_* stuff in the r8169 driver. :o/
> 
> So, yes, ignore this point.
> 
>>> Btw it's a bit unexpected to see a "Fixes" tag related to a RTL8125 bug as
>>> well as a "Tested-by" by the bugzilla submitter when the dmesg included in
>>> bz216157 exibits a RTL8168e/8111e.
>>>
>> The Fixes tag refers to the latest change to the affected code, therefore
>> it comes a little unexpected, right.
> 
> ?
> 
> 8d520b4de3ed does not change the affected code.
> 

This chunk of 8d520b4de3ed 

@@ -4128,9 +4183,10 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 
 		opts[1] |= transport_offset << TCPHO_SHIFT;
 	} else {
-		if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
-			/* eth_skb_pad would free the skb on error */
-			return !__skb_put_padto(skb, ETH_ZLEN, false);
+		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
+
+		/* skb_padto would free the skb on error */
+		return !__skb_put_padto(skb, padto, false);
 	}
 
 	return true;

changes the context for this part of the patch. Therefore the patch wouldn't
apply cleanly.


@@ -4235,7 +4234,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 		else
 			WARN_ON_ONCE(1);
 
-		opts[1] |= transport_offset << TCPHO_SHIFT;
+		opts[1] |= skb_transport_offset(skb) << TCPHO_SHIFT;
 	} else {
 		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
 


> Eric's unset transport offset detection debug code would have produced the
> same output with the parent of the "Fixes" commit id:
> 

I know, but due to the fact that the warnings are harmless and the new check
doesn't exist in earlier versions, I think we can omit these kernel versions.

> $ git cat-file -p 8d520b4de3ed^:drivers/net/ethernet/realtek/r8169_main.c | grep -A4 -B1 -E 'rtl8169_features_check'
> 
> static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
> 						struct net_device *dev,
> 						netdev_features_t features)
> {
> 	int transport_offset = skb_transport_offset(skb);
> --
> 	.ndo_start_xmit		= rtl8169_start_xmit,
> 	.ndo_features_check	= rtl8169_features_check,
> 	.ndo_tx_timeout		= rtl8169_tx_timeout,
> 	.ndo_validate_addr	= eth_validate_addr,
> 	.ndo_change_mtu		= rtl8169_change_mtu,
> 	.ndo_fix_features	= rtl8169_fix_features,
> 
> 
> -> 8d520b4de3ed does not modify the first
> 'int transport_offset = skb_transport_offset(skb);' statement and neither
> does it modify the code path to rtl8169_features_check 
> 
> 8d520b4de3ed actually removes some logical path towards rtl8169_tso_csum_v2
> but it does not change (nor does it break) the relevant code:
> 
> $ git cat-file -p 8d520b4de3ed^:drivers/net/ethernet/realtek/r8169_main.c | grep -A3 -B1 -E 'bool rtl8169_tso_csum_v2'
> 
> static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
> 				struct sk_buff *skb, u32 *opts)
> {
> 	u32 transport_offset = (u32)skb_transport_offset(skb);
> 
> 

Re: [PATCH net] r8169: fix accessing unset transport header

[Paolo Abeni]

On Mon, 2022-07-04 at 00:12 +0200, Heiner Kallweit wrote:
> 66e4c8d95008 ("net: warn if transport header was not set") added
> a check that triggers a warning in r8169, see [0].
> 
> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216157
> 
> Fixes: 8d520b4de3ed ("r8169: work around RTL8125 UDP hw bug")
> Reported-by: Erhard F. <erhard_f@mailbox.org>
> Tested-by: Erhard F. <erhard_f@mailbox.org>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

The patch LGTM, but I think you could mention in the commit message
that the bug was [likely] introduced with commit bdfa4ed68187 ("r8169:
use Giant Send"), but this change applies only on top of the commit
specified by the fixes tag - just to help stable teams.

Thanks!

Paolo

> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 3098d6672..1b7fdb4f0 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -4190,7 +4190,6 @@ static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)
>  static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>  				struct sk_buff *skb, u32 *opts)
>  {
> -	u32 transport_offset = (u32)skb_transport_offset(skb);
>  	struct skb_shared_info *shinfo = skb_shinfo(skb);
>  	u32 mss = shinfo->gso_size;
>  
> @@ -4207,7 +4206,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>  			WARN_ON_ONCE(1);
>  		}
>  
> -		opts[0] |= transport_offset << GTTCPHO_SHIFT;
> +		opts[0] |= skb_transport_offset(skb) << GTTCPHO_SHIFT;
>  		opts[1] |= mss << TD1_MSS_SHIFT;
>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>  		u8 ip_protocol;
> @@ -4235,7 +4234,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>  		else
>  			WARN_ON_ONCE(1);
>  
> -		opts[1] |= transport_offset << TCPHO_SHIFT;
> +		opts[1] |= skb_transport_offset(skb) << TCPHO_SHIFT;
>  	} else {
>  		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
>  
> @@ -4402,14 +4401,13 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>  						struct net_device *dev,
>  						netdev_features_t features)
>  {
> -	int transport_offset = skb_transport_offset(skb);
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  
>  	if (skb_is_gso(skb)) {
>  		if (tp->mac_version == RTL_GIGA_MAC_VER_34)
>  			features = rtl8168evl_fix_tso(skb, features);
>  
> -		if (transport_offset > GTTCPHO_MAX &&
> +		if (skb_transport_offset(skb) > GTTCPHO_MAX &&
>  		    rtl_chip_supports_csum_v2(tp))
>  			features &= ~NETIF_F_ALL_TSO;
>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
> @@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>  		if (rtl_quirk_packet_padto(tp, skb))
>  			features &= ~NETIF_F_CSUM_MASK;
>  
> -		if (transport_offset > TCPHO_MAX &&
> +		if (skb_transport_offset(skb) > TCPHO_MAX &&
>  		    rtl_chip_supports_csum_v2(tp))
>  			features &= ~NETIF_F_CSUM_MASK;
>  	}

Re: [PATCH net] r8169: fix accessing unset transport header

[Heiner Kallweit]

On 05.07.2022 14:46, Paolo Abeni wrote:
> On Mon, 2022-07-04 at 00:12 +0200, Heiner Kallweit wrote:
>> 66e4c8d95008 ("net: warn if transport header was not set") added
>> a check that triggers a warning in r8169, see [0].
>>
>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216157
>>
>> Fixes: 8d520b4de3ed ("r8169: work around RTL8125 UDP hw bug")
>> Reported-by: Erhard F. <erhard_f@mailbox.org>
>> Tested-by: Erhard F. <erhard_f@mailbox.org>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> The patch LGTM, but I think you could mention in the commit message
> that the bug was [likely] introduced with commit bdfa4ed68187 ("r8169:
> use Giant Send"), but this change applies only on top of the commit
> specified by the fixes tag - just to help stable teams.
> 
Right, I'll submit a v2 with more details in the commit message.

> Thanks!
> 
> Paolo
> 
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index 3098d6672..1b7fdb4f0 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -4190,7 +4190,6 @@ static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)
>>  static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>>  				struct sk_buff *skb, u32 *opts)
>>  {
>> -	u32 transport_offset = (u32)skb_transport_offset(skb);
>>  	struct skb_shared_info *shinfo = skb_shinfo(skb);
>>  	u32 mss = shinfo->gso_size;
>>  
>> @@ -4207,7 +4206,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>>  			WARN_ON_ONCE(1);
>>  		}
>>  
>> -		opts[0] |= transport_offset << GTTCPHO_SHIFT;
>> +		opts[0] |= skb_transport_offset(skb) << GTTCPHO_SHIFT;
>>  		opts[1] |= mss << TD1_MSS_SHIFT;
>>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>  		u8 ip_protocol;
>> @@ -4235,7 +4234,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>>  		else
>>  			WARN_ON_ONCE(1);
>>  
>> -		opts[1] |= transport_offset << TCPHO_SHIFT;
>> +		opts[1] |= skb_transport_offset(skb) << TCPHO_SHIFT;
>>  	} else {
>>  		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
>>  
>> @@ -4402,14 +4401,13 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>>  						struct net_device *dev,
>>  						netdev_features_t features)
>>  {
>> -	int transport_offset = skb_transport_offset(skb);
>>  	struct rtl8169_private *tp = netdev_priv(dev);
>>  
>>  	if (skb_is_gso(skb)) {
>>  		if (tp->mac_version == RTL_GIGA_MAC_VER_34)
>>  			features = rtl8168evl_fix_tso(skb, features);
>>  
>> -		if (transport_offset > GTTCPHO_MAX &&
>> +		if (skb_transport_offset(skb) > GTTCPHO_MAX &&
>>  		    rtl_chip_supports_csum_v2(tp))
>>  			features &= ~NETIF_F_ALL_TSO;
>>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> @@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>>  		if (rtl_quirk_packet_padto(tp, skb))
>>  			features &= ~NETIF_F_CSUM_MASK;
>>  
>> -		if (transport_offset > TCPHO_MAX &&
>> +		if (skb_transport_offset(skb) > TCPHO_MAX &&
>>  		    rtl_chip_supports_csum_v2(tp))
>>  			features &= ~NETIF_F_CSUM_MASK;
>>  	}
> 

Re: [PATCH net] r8169: fix accessing unset transport header

[Heiner Kallweit]

On 05.07.2022 21:12, Jakub Kicinski wrote:
> On Tue, 05 Jul 2022 14:46:14 +0200 Paolo Abeni wrote:
>> On Mon, 2022-07-04 at 00:12 +0200, Heiner Kallweit wrote:
>>> 66e4c8d95008 ("net: warn if transport header was not set") added
>>> a check that triggers a warning in r8169, see [0].
>>>
>>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216157
>>>
>>> Fixes: 8d520b4de3ed ("r8169: work around RTL8125 UDP hw bug")
>>> Reported-by: Erhard F. <erhard_f@mailbox.org>
>>> Tested-by: Erhard F. <erhard_f@mailbox.org>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>  
>>
>> The patch LGTM, but I think you could mention in the commit message
>> that the bug was [likely] introduced with commit bdfa4ed68187 ("r8169:
>> use Giant Send"), but this change applies only on top of the commit
>> specified by the fixes tag - just to help stable teams.
> 
> How about we put Eric's patch under Fixes? The patch is not really
> needed unless the warning is there.

This would also be an option. It just seemed a little illogical to me
to leave the impression a new (useful) warning needs to be fixed.
Just a few seconds ago I sent a v2 following Paolo's proposal.

Re: [PATCH net] r8169: fix accessing unset transport header

[Jakub Kicinski]

On Tue, 5 Jul 2022 21:18:43 +0200 Heiner Kallweit wrote:
> > How about we put Eric's patch under Fixes? The patch is not really
> > needed unless the warning is there.  
> 
> This would also be an option. It just seemed a little illogical to me
> to leave the impression a new (useful) warning needs to be fixed.
> Just a few seconds ago I sent a v2 following Paolo's proposal.

That's fine. Perhaps I have more utilitarian than literal view of the
Fixes tag than others (how far back can the problem trigger vs blame).