* [PATCH net] r8169: fix accessing unset transport header
@ 2022-07-03 22:12 Heiner Kallweit
2022-07-04 0:55 ` Francois Romieu
2022-07-05 12:46 ` Paolo Abeni
0 siblings, 2 replies; 9+ messages in thread
From: Heiner Kallweit @ 2022-07-03 22:12 UTC (permalink / raw)
To: Jakub Kicinski, David Miller, Realtek linux nic maintainers
Cc: netdev, Erhard F.
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
2022-07-03 22:12 [PATCH net] r8169: fix accessing unset transport header Heiner Kallweit
@ 2022-07-04 0:55 ` Francois Romieu
2022-07-04 9:15 ` Heiner Kallweit
2022-07-05 12:46 ` Paolo Abeni
1 sibling, 1 reply; 9+ messages in thread
From: Francois Romieu @ 2022-07-04 0:55 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Jakub Kicinski, David Miller, Realtek linux nic maintainers,
netdev, Erhard F.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
2022-07-04 0:55 ` Francois Romieu
@ 2022-07-04 9:15 ` Heiner Kallweit
2022-07-04 15:40 ` Francois Romieu
0 siblings, 1 reply; 9+ messages in thread
From: Heiner Kallweit @ 2022-07-04 9:15 UTC (permalink / raw)
To: Francois Romieu
Cc: Jakub Kicinski, David Miller, Realtek linux nic maintainers,
netdev, Erhard F.
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
2022-07-04 9:15 ` Heiner Kallweit
@ 2022-07-04 15:40 ` Francois Romieu
2022-07-04 19:10 ` Heiner Kallweit
0 siblings, 1 reply; 9+ messages in thread
From: Francois Romieu @ 2022-07-04 15:40 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Jakub Kicinski, David Miller, Realtek linux nic maintainers,
netdev, Erhard F.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
2022-07-04 15:40 ` Francois Romieu
@ 2022-07-04 19:10 ` Heiner Kallweit
0 siblings, 0 replies; 9+ messages in thread
From: Heiner Kallweit @ 2022-07-04 19:10 UTC (permalink / raw)
To: Francois Romieu
Cc: Jakub Kicinski, David Miller, Realtek linux nic maintainers,
netdev, Erhard F.
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);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
2022-07-03 22:12 [PATCH net] r8169: fix accessing unset transport header Heiner Kallweit
2022-07-04 0:55 ` Francois Romieu
@ 2022-07-05 12:46 ` Paolo Abeni
2022-07-05 19:11 ` Heiner Kallweit
[not found] ` <20220705121230.69a4e0e8@kernel.org>
1 sibling, 2 replies; 9+ messages in thread
From: Paolo Abeni @ 2022-07-05 12:46 UTC (permalink / raw)
To: Heiner Kallweit, Jakub Kicinski, David Miller,
Realtek linux nic maintainers
Cc: netdev, Erhard F.
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;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
2022-07-05 12:46 ` Paolo Abeni
@ 2022-07-05 19:11 ` Heiner Kallweit
[not found] ` <20220705121230.69a4e0e8@kernel.org>
1 sibling, 0 replies; 9+ messages in thread
From: Heiner Kallweit @ 2022-07-05 19:11 UTC (permalink / raw)
To: Paolo Abeni, Jakub Kicinski, David Miller, Realtek linux nic maintainers
Cc: netdev, Erhard F.
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;
>> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
[not found] ` <20220705121230.69a4e0e8@kernel.org>
@ 2022-07-05 19:18 ` Heiner Kallweit
2022-07-05 19:23 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Heiner Kallweit @ 2022-07-05 19:18 UTC (permalink / raw)
To: Jakub Kicinski, Paolo Abeni
Cc: David Miller, Realtek linux nic maintainers, netdev, Erhard F."
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] r8169: fix accessing unset transport header
2022-07-05 19:18 ` Heiner Kallweit
@ 2022-07-05 19:23 ` Jakub Kicinski
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2022-07-05 19:23 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Paolo Abeni, David Miller, Realtek linux nic maintainers, netdev
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).
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-07-05 19:23 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-03 22:12 [PATCH net] r8169: fix accessing unset transport header Heiner Kallweit
2022-07-04 0:55 ` Francois Romieu
2022-07-04 9:15 ` Heiner Kallweit
2022-07-04 15:40 ` Francois Romieu
2022-07-04 19:10 ` Heiner Kallweit
2022-07-05 12:46 ` Paolo Abeni
2022-07-05 19:11 ` Heiner Kallweit
[not found] ` <20220705121230.69a4e0e8@kernel.org>
2022-07-05 19:18 ` Heiner Kallweit
2022-07-05 19:23 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.