netdev - Re: [PATCH net-next 3/3] virtio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250113140446.12d7b7d3@kernel.org>
Date: Mon, 13 Jan 2025 14:04:46 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Joe Damato <jdamato@...tly.com>
Cc: Jason Wang <jasowang@...hat.com>, netdev@...r.kernel.org,
 mkarsten@...terloo.ca, "Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo
 <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>, Andrew Lunn <andrew+netdev@...n.ch>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo
 Abeni <pabeni@...hat.com>, "open list:VIRTIO CORE AND NET DRIVERS"
 <virtualization@...ts.linux.dev>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 3/3] virtio_net: Map NAPIs to queues

On Mon, 13 Jan 2025 09:30:20 -0800 Joe Damato wrote:
> > >  static void virtnet_napi_enable_lock(struct virtqueue *vq,
> > > -                                    struct napi_struct *napi)
> > > +                                    struct napi_struct *napi,
> > > +                                    bool need_rtnl)
> > >  {
> > > +       struct virtnet_info *vi = vq->vdev->priv;
> > > +       int q = vq2rxq(vq);
> > > +
> > >         virtnet_napi_do_enable(vq, napi);
> > > +
> > > +       if (q < vi->curr_queue_pairs) {
> > > +               if (need_rtnl)
> > > +                       rtnl_lock();  
> > 
> > Can we tweak the caller to call rtnl_lock() instead to avoid this trick?  
> 
> The major problem is that if the caller calls rtnl_lock() before
> calling virtnet_napi_enable_lock, then virtnet_napi_do_enable (and
> thus napi_enable) happen under the lock.
> 
> Jakub mentioned in a recent change [1] that napi_enable may soon
> need to sleep.
> 
> Given the above constraints, the only way to avoid the "need_rtnl"
> would be to refactor the code much more, placing calls (or wrappers)
> to netif_queue_set_napi in many locations.
> 
> IMHO: This implementation seemed cleaner than putting calls to
> netif_queue_set_napi throughout the driver.
> 
> Please let me know how you'd like to proceed on this.
> 
> [1]: https://lore.kernel.org/netdev/20250111024742.3680902-1-kuba@kernel.org/

I'm going to make netif_queue_set_napi() take netdev->lock, and remove
the rtnl_lock requirement ~this week. If we need conditional locking
perhaps we're better off waiting?