Re: [PATCH net v7 1/4] net: hsr: keep GRO disabled on HSR/PRP ports

[Xin Xie <xiexinet@gmail.com>]

> This looks better than what I remember from the previous version. Are
> the bot's complains legitime?

Yes, the Critical one is real.

veth creates and deletes its NAPI instances based on wanted_features
(veth_gro_requested() in veth_open() and veth_close()). This patch only
clears NETIF_F_GRO from the active features. So after the attach,
wanted_features still says "GRO on" while the feature is off.

Two things can go wrong because of that:

- The member is up with GRO on, then gets attached. veth_set_features()
  deletes the NAPIs. Later the member is set down and veth_close()
  deletes them a second time. napi_disable() then waits forever.

- The member is down with GRO on, gets attached, is set up, and is then
  detached while still up. veth_open() created the NAPIs, and the
  detach creates them again. That ends in the BUG_ON() in napi_enable().

The v7 selftests always attach and detach the member while it is up, so
they did not see either case. I already replied to the bot with
"pw-bot: cr", but by mistake under the patch 4 report.

My plan for v8: keep the persistent filter, but at the same place also
clear GRO from the member's wanted_features. This is what
netdev_sync_lower_features() does for the UPPER_DISABLES bits. Then
requested and active GRO always agree, and veth does not need a change.
The downside: after the member is detached, GRO stays off, and the user
has to enable it again. LRO behaves the same way today.

If you or the other maintainers prefer that veth is fixed instead, so
that it looks at the active feature, I can do that.

The Low one is only a comment problem. For stacked members (bond,
macvlan, ...) GRO runs on the lower device, so "software GRO must be
off before frames can arrive" is only true for direct members. Patch 3
segments any GSO input anyway. I will fix the comment.

-- 
Xin