Re: [PATCH net-next v9 1/7] net: rtnetlink: add pacing_offload attribute to net_device

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1 (1)
> 
> - [Medium] `pacing_offload:1` is added as the first *runtime-mutable* bit
>   of the `struct_group(priv_flags_fast, ...)` bitfield word in struct…
> - [Low] Concern (verified as NOT a defect): dev->pacing_offload /
>   IFLA_PACING_OFFLOAD is claimed to have no in-tree consumer at this…

TL;DR: I think all reports to this series are false positives.

I'll respond in detail, skipping only some too obvious low ones
(e.g., "this feature is not used yet, but will be in a next patch").
 
> Pre-existing issues:
> - [Low] Pre-existing YAML spec gap adjacent to the change: the shared
>   getlink do/dump reply list (`&link-all-attrs`,…
> 
> --
> 
> net: rtnetlink: add pacing_offload attribute to net_device
> 
> This adds a new IFLA_PACING_OFFLOAD link attribute plus a matching
> pacing_offload:1 bitfield in struct net_device, so pacing offload can be
> turned on and off administratively.  It also marks
> IFLA_MAX_PACING_OFFLOAD_HORIZON as NLA_REJECT and documents the new
> attribute in the rt-link YAML spec.
> 
> A few questions below.
> 
> > diff --git a/Documentation/netlink/specs/rt-link.yaml b/Documentation/netlink/specs/rt-link.yaml
> > index 5217d0bb4a3a5..7a72cd1b7e1ee 100644
> > --- a/Documentation/netlink/specs/rt-link.yaml
> > +++ b/Documentation/netlink/specs/rt-link.yaml
> > @@ -1091,6 +1091,10 @@ attribute-sets:
> >        -
> >          name: tailroom
> >          type: u16
> > +      -
> > +        name: pacing-offload
> > +        type: u32
> > +        doc: Enable EDT pacing offload (0 - disabled, 1 - enabled).
> >    -
> >      name: prop-list-link-attrs
> >      subset-of: link-attrs
> > @@ -2559,6 +2563,7 @@ operations:
> >              - devlink-port
> >              - gso-ipv4-max-size
> >              - gro-ipv4-max-size
> > +            - pacing-offload
> 
> [Severity: Low]
> This isn't something introduced by this patch, but while the shared
> getlink reply list (&link-all-attrs) is being extended here, should
> max-pacing-offload-horizon be added as well?
> 
> rtnl_fill_ifinfo() emits it unconditionally in the same nla_put chain as
> the new attribute:
> 
> 	    nla_put_uint(skb, IFLA_MAX_PACING_OFFLOAD_HORIZON,
> 			 READ_ONCE(dev->max_pacing_offload_horizon)) ||
> 
> The same holds for netns-immutable, headroom and tailroom, which are also
> always emitted but not listed in the reply set.
> 
> The commit message advertises reading the horizon with
> 
>     ynl --family rt-link --do getlink ... | grep pacing
> 
> which works, because the Python decoder resolves reply members from
> attribute-set: link-attrs, but consumers generated from the per-operation
> reply list would not see these members.

This patch follows the established behavior of other recent read-only
rt-link attrs.

It's a limitation of how get-link and set-link use the same attrs.

This is a damned if you do, damned if you don't issue where the bot
will call you out either way.

> > diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
> > index 512f6d6fa3d8f..8eceaa80b6860 100644
> > --- a/Documentation/networking/net_cachelines/net_device.rst
> > +++ b/Documentation/networking/net_cachelines/net_device.rst
> > @@ -11,6 +11,7 @@ Type                                Name                        fastpath_tx_acce
> >  unsigned_long:32                    priv_flags                  read_mostly                             __dev_queue_xmit(tx)
> >  unsigned_long:1                     lltx                        read_mostly                             HARD_TX_LOCK,HARD_TX_TRYLOCK,HARD_TX_UNLOCK(tx)
> >  unsigned_long:2                     netmem_tx:2;                read_mostly
> > +unsigned_long:1                     pacing_offload              read_mostly                             sch_fq
> >  char                                name[16]
> >  struct netdev_name_node*            name_node
> >  struct dev_ifalias*                 ifalias
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 707b2e51c2b97..1f0710eef185b 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> 
> [ ... ]
> 
> > @@ -2170,6 +2171,7 @@ struct net_device {
> >  		unsigned long		priv_flags:32;
> >  		unsigned long		lltx:1;
> >  		unsigned long		netmem_tx:2;
> > +		unsigned long		pacing_offload:1;
> >  	);
> 
> [Severity: Medium]
> This puts the first runtime-mutable bit into the priv_flags_fast group.  On
> 64-bit, priv_flags, lltx, netmem_tx and pacing_offload all live in one
> unsigned long, and adjacent bitfields are a single memory location, so a
> store to one of them is a read-modify-write of the others.
> 
> do_setlink() writes the new bit under RTNL only:
> 
> 		if (dev->pacing_offload != val) {
> 			dev->pacing_offload = val;
> 
> but priv_flags in the same word is modified from softirq context with no
> RTNL held, in net/ncsi/ncsi-rsp.c:
> 
> 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> 
> That appears in ncsi_rsp_handler_oem_bcm() and ncsi_rsp_handler_gmcma(),
> both reached from ncsi_rcv_rsp(), which is a packet_type receive handler.

The bug here is in ncsi_ imho.

The other fields are written at net_device init and immutable
otherwise. The rule for these fields is immutable or RTNL held, AFAIK.

> Can these two read-modify-writes lose each other's update, so that either
> IFF_LIVE_ADDR_CHANGE is silently dropped, or pacing_offload reads back 0
> after do_setlink() already set DO_SETLINK_MODIFIED and sent a notification?
> 
> A second point on the same field: a 1-bit bitfield cannot be accessed with
> READ_ONCE()/WRITE_ONCE(), yet the net_cachelines row added above documents
> pacing_offload as a read_mostly field consumed lockless by sch_fq.  Every
> neighbouring netdev field in these paths is accessed with
> READ_ONCE()/WRITE_ONCE(), including max_pacing_offload_horizon right next
> to it.
> 
> Would a plain bool/u8, or a dedicated unsigned long used with
> set_bit()/test_bit(), work here instead?  That would make the update
> independent of the neighbouring bits and allow marked lockless reads.