[PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Tariq Toukan]

Calling netdev_increment_features() on upper/master device from
netdev_add_tso_features() implies unintentional clearance of ALL_FOR_ALL
features supported by all slaves.  Fix it by passing ALL_FOR_ALL in
addition to ALL_TSO.

Fixes: b0ce3508b25e ("bonding: allow TSO being set on bonding master")
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 include/linux/netdevice.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi,

I know that netdev_increment_features() does not set any feature that's
unmasked in the mask argument.
I wonder why it can clear them though, was it meant to be like this?
If not, then the proper fix should be in netdev_increment_features(), not
in netdev_add_tso_features().


diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 18dec08439f9..a9d5e4bb829b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4748,7 +4748,7 @@ netdev_features_t netdev_increment_features(netdev_features_t all,
 static inline netdev_features_t netdev_add_tso_features(netdev_features_t features,
 							netdev_features_t mask)
 {
-	return netdev_increment_features(features, NETIF_F_ALL_TSO, mask);
+	return netdev_increment_features(features, NETIF_F_ALL_TSO | NETIF_F_ALL_FOR_ALL, mask);
 }
 
 int __netdev_update_features(struct net_device *dev);
-- 
2.21.0

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Eric Dumazet]

On Mon, Nov 23, 2020 at 3:13 PM Tariq Toukan <tariqt@nvidia.com> wrote:
>
> Calling netdev_increment_features() on upper/master device from
> netdev_add_tso_features() implies unintentional clearance of ALL_FOR_ALL
> features supported by all slaves.  Fix it by passing ALL_FOR_ALL in
> addition to ALL_TSO.
>
> Fixes: b0ce3508b25e ("bonding: allow TSO being set on bonding master")

I think you should give more details to your bug report, because
netdev_add_tso_features() is used from different
places.

Thanks.

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Tariq Toukan]

On 11/23/2020 4:55 PM, Eric Dumazet wrote:
> On Mon, Nov 23, 2020 at 3:13 PM Tariq Toukan <tariqt@nvidia.com> wrote:
>>
>> Calling netdev_increment_features() on upper/master device from
>> netdev_add_tso_features() implies unintentional clearance of ALL_FOR_ALL
>> features supported by all slaves.  Fix it by passing ALL_FOR_ALL in
>> addition to ALL_TSO.
>>
>> Fixes: b0ce3508b25e ("bonding: allow TSO being set on bonding master")
> 
> I think you should give more details to your bug report, because
> netdev_add_tso_features() is used from different
> places.
> 
> Thanks.
> 

Right. I'll include these in the re-spin:
Fixes: 247f6d0f8667 ("team: allow TSO being set on master")
Fixes: f902e8812ef6 ("bridge: Add ability to enable TSO")

I wonder though if netdev_increment_features() is expected to clear 
features that are not part of the mask.

Thanks.

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Eric Dumazet]

On Mon, Nov 23, 2020 at 5:15 PM Tariq Toukan <ttoukan.linux@gmail.com> wrote:
>
>
>
> On 11/23/2020 4:55 PM, Eric Dumazet wrote:
> > On Mon, Nov 23, 2020 at 3:13 PM Tariq Toukan <tariqt@nvidia.com> wrote:
> >>
> >> Calling netdev_increment_features() on upper/master device from
> >> netdev_add_tso_features() implies unintentional clearance of ALL_FOR_ALL
> >> features supported by all slaves.  Fix it by passing ALL_FOR_ALL in
> >> addition to ALL_TSO.
> >>
> >> Fixes: b0ce3508b25e ("bonding: allow TSO being set on bonding master")
> >
> > I think you should give more details to your bug report, because
> > netdev_add_tso_features() is used from different
> > places.
> >
> > Thanks.
> >
>
> Right. I'll include these in the re-spin:
> Fixes: 247f6d0f8667 ("team: allow TSO being set on master")
> Fixes: f902e8812ef6 ("bridge: Add ability to enable TSO")

I was more thinking about what exact issue you had, and how we can
reproduce it, and test the fix.

>
> I wonder though if netdev_increment_features() is expected to clear
> features that are not part of the mask.

Well, the 'increment' part was suggesting the function was adding
flags, not removing them.

We might ask Herbert Xu if we :

1) Need to comment the function, or change its name to be more descriptive.
2) Change the behavior (as you suggested)
3) Other choice.

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Tariq Toukan]

On 11/24/2020 12:48 PM, Eric Dumazet wrote:
> On Mon, Nov 23, 2020 at 5:15 PM Tariq Toukan <ttoukan.linux@gmail.com> wrote:
>>
>>
>>
>> On 11/23/2020 4:55 PM, Eric Dumazet wrote:
>>> On Mon, Nov 23, 2020 at 3:13 PM Tariq Toukan <tariqt@nvidia.com> wrote:
>>>>
>>>> Calling netdev_increment_features() on upper/master device from
>>>> netdev_add_tso_features() implies unintentional clearance of ALL_FOR_ALL
>>>> features supported by all slaves.  Fix it by passing ALL_FOR_ALL in
>>>> addition to ALL_TSO.
>>>>
>>>> Fixes: b0ce3508b25e ("bonding: allow TSO being set on bonding master")
>>>
>>> I think you should give more details to your bug report, because
>>> netdev_add_tso_features() is used from different
>>> places.
>>>
>>> Thanks.
>>>
>>
>> Right. I'll include these in the re-spin:
>> Fixes: 247f6d0f8667 ("team: allow TSO being set on master")
>> Fixes: f902e8812ef6 ("bridge: Add ability to enable TSO")
> 
> I was more thinking about what exact issue you had, and how we can
> reproduce it, and test the fix.
> 

Issue reproduction is very simple:
Pick any of the features under ALL_FOR_ALL, like tx-nocache-copy.
Turn it on for all slaves.
Turn it on for the bond.
You'll still not be able to use it:
     tx-nocache-copy: off [requested on]

Reason is that the call to netdev_add_tso_features() being considered as 
a "dummy" slave that has this feature bit cleared, breaking ALL_FOR_ALL 
logic.

>>
>> I wonder though if netdev_increment_features() is expected to clear
>> features that are not part of the mask.
> 
> Well, the 'increment' part was suggesting the function was adding
> flags, not removing them.
> 

Yes, that's confusing... Although ALL_FOR_ALL logic is just about 
removing, unlike ONE_FOR_ALL.

> We might ask Herbert Xu if we :
> 
> 1) Need to comment the function, or change its name to be more descriptive.
> 2) Change the behavior (as you suggested)
> 3) Other choice.
> 

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Herbert Xu]

On Tue, Nov 24, 2020 at 11:48:35AM +0100, Eric Dumazet wrote:
>
> Well, the 'increment' part was suggesting the function was adding
> flags, not removing them.

The idea of the increment part is that we're adding a constituent
device, not that we're adding features.  There have always been
features which were conjunctions, i.e., they must be supported by
all underlying devices for them to be enabled on the virtual device.

Your use of the increment function is unusual, as you're not adding
features that belong to one underlying device, but rather you're
trying to enable a feature on the virtual device unconditionally.

> We might ask Herbert Xu if we :
> 
> 1) Need to comment the function, or change its name to be more descriptive.
> 2) Change the behavior (as you suggested)
> 3) Other choice.

I think Tariq's patch is fine, although a comment should be added
to netdev_add_tso_features as this use of the increment function
is nonstandard.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Tariq Toukan]

On 11/25/2020 5:25 AM, Herbert Xu wrote:
> On Tue, Nov 24, 2020 at 11:48:35AM +0100, Eric Dumazet wrote:
>>
>> Well, the 'increment' part was suggesting the function was adding
>> flags, not removing them.
> 
> The idea of the increment part is that we're adding a constituent
> device, not that we're adding features.  There have always been
> features which were conjunctions, i.e., they must be supported by
> all underlying devices for them to be enabled on the virtual device.
> 
> Your use of the increment function is unusual, as you're not adding
> features that belong to one underlying device, but rather you're
> trying to enable a feature on the virtual device unconditionally.
> 
>> We might ask Herbert Xu if we :
>>
>> 1) Need to comment the function, or change its name to be more descriptive.
>> 2) Change the behavior (as you suggested)
>> 3) Other choice.
> 
> I think Tariq's patch is fine, although a comment should be added
> to netdev_add_tso_features as this use of the increment function
> is nonstandard.
> 

Thanks Herbert, I'll add a comment and re-spin.

> Thanks,
> 

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Eric Dumazet]

On Wed, Nov 25, 2020 at 10:06 AM Tariq Toukan <ttoukan.linux@gmail.com> wrote:
>
>
>
> On 11/25/2020 5:25 AM, Herbert Xu wrote:
> > On Tue, Nov 24, 2020 at 11:48:35AM +0100, Eric Dumazet wrote:
> >>
> >> Well, the 'increment' part was suggesting the function was adding
> >> flags, not removing them.
> >
> > The idea of the increment part is that we're adding a constituent
> > device, not that we're adding features.  There have always been
> > features which were conjunctions, i.e., they must be supported by
> > all underlying devices for them to be enabled on the virtual device.
> >
> > Your use of the increment function is unusual, as you're not adding
> > features that belong to one underlying device, but rather you're
> > trying to enable a feature on the virtual device unconditionally.

This was not the intent.

We can still disable TSO on the bonding device if desired.

pk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
tcp-segmentation-offload; done
tcp-segmentation-offload: on
tcp-segmentation-offload: on
tcp-segmentation-offload: on
lpk51:~# ethtool -K bond0 tso off
Actual changes:
tcp-segmentation-offload: off
tx-tcp-segmentation: off
tx-tcp-ecn-segmentation: off
tx-tcp-mangleid-segmentation: off
tx-tcp6-segmentation: off
large-receive-offload: off [requested on]
lpk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
tcp-segmentation-offload; done
tcp-segmentation-offload: off
tcp-segmentation-offload: on
tcp-segmentation-offload: on

The intent was that we could have :

lpk51:~# ethtool -K bond0 tso on
Actual changes:
tcp-segmentation-offload: on
tx-tcp-segmentation: on
tx-tcp-ecn-segmentation: on
tx-tcp-mangleid-segmentation: on
tx-tcp6-segmentation: on
lpk51:~# ethtool -K eth1 tso off
lpk51:~# ethtool -K eth2 tso off
lpk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
tcp-segmentation-offload; done
tcp-segmentation-offload: on
tcp-segmentation-offload: off
tcp-segmentation-offload: off
lpk51:~#


> >
> >> We might ask Herbert Xu if we :
> >>
> >> 1) Need to comment the function, or change its name to be more descriptive.
> >> 2) Change the behavior (as you suggested)
> >> 3) Other choice.
> >
> > I think Tariq's patch is fine, although a comment should be added
> > to netdev_add_tso_features as this use of the increment function
> > is nonstandard.
> >
>
> Thanks Herbert, I'll add a comment and re-spin.

I think we should remove the use of  netdev_increment_features() and
replace it with something else,
because there is too much confusion.

Re: [PATCH net] netdevice.h: Fix unintentional disable of ALL_FOR_ALL features on upper device

[Tariq Toukan]

On 11/25/2020 11:27 AM, Eric Dumazet wrote:
> On Wed, Nov 25, 2020 at 10:06 AM Tariq Toukan <ttoukan.linux@gmail.com> wrote:
>>
>>
>>
>> On 11/25/2020 5:25 AM, Herbert Xu wrote:
>>> On Tue, Nov 24, 2020 at 11:48:35AM +0100, Eric Dumazet wrote:
>>>>
>>>> Well, the 'increment' part was suggesting the function was adding
>>>> flags, not removing them.
>>>
>>> The idea of the increment part is that we're adding a constituent
>>> device, not that we're adding features.  There have always been
>>> features which were conjunctions, i.e., they must be supported by
>>> all underlying devices for them to be enabled on the virtual device.
>>>
>>> Your use of the increment function is unusual, as you're not adding
>>> features that belong to one underlying device, but rather you're
>>> trying to enable a feature on the virtual device unconditionally.
> 
> This was not the intent.
> 
> We can still disable TSO on the bonding device if desired.
> 
> pk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
> tcp-segmentation-offload; done
> tcp-segmentation-offload: on
> tcp-segmentation-offload: on
> tcp-segmentation-offload: on
> lpk51:~# ethtool -K bond0 tso off
> Actual changes:
> tcp-segmentation-offload: off
> tx-tcp-segmentation: off
> tx-tcp-ecn-segmentation: off
> tx-tcp-mangleid-segmentation: off
> tx-tcp6-segmentation: off
> large-receive-offload: off [requested on]
> lpk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
> tcp-segmentation-offload; done
> tcp-segmentation-offload: off
> tcp-segmentation-offload: on
> tcp-segmentation-offload: on
> 
> The intent was that we could have :
> 
> lpk51:~# ethtool -K bond0 tso on
> Actual changes:
> tcp-segmentation-offload: on
> tx-tcp-segmentation: on
> tx-tcp-ecn-segmentation: on
> tx-tcp-mangleid-segmentation: on
> tx-tcp6-segmentation: on
> lpk51:~# ethtool -K eth1 tso off
> lpk51:~# ethtool -K eth2 tso off
> lpk51:~# for i in bond0 eth1 eth2; do ethtool -k $i|grep
> tcp-segmentation-offload; done
> tcp-segmentation-offload: on
> tcp-segmentation-offload: off
> tcp-segmentation-offload: off
> lpk51:~#
> 
> 

IIUC, we want to let the bond TSO feature bit be totally independent, 
not affected by slaves.
If so, I think that:
First we should take NETIF_F_GSO_SOFTWARE (or just NETIF_F_ALL_TSO) out 
of NETIF_F_ONE_FOR_ALL.
Then, make sure it is set in bond_setup (it is already done, as part of 
BOND_VLAN_FEATURES).

I think this new logic is good for all other upper devices, which will 
be affected by the change in NETIF_F_ONE_FOR_ALL.


>>>
>>>> We might ask Herbert Xu if we :
>>>>
>>>> 1) Need to comment the function, or change its name to be more descriptive.
>>>> 2) Change the behavior (as you suggested)
>>>> 3) Other choice.
>>>
>>> I think Tariq's patch is fine, although a comment should be added
>>> to netdev_add_tso_features as this use of the increment function
>>> is nonstandard.
>>>
>>
>> Thanks Herbert, I'll add a comment and re-spin.
> 
> I think we should remove the use of  netdev_increment_features() and
> replace it with something else,
> because there is too much confusion.
> 

I think it would be best.

I can prepare the patch I described above if you agree with it.