netdev - Re: [PATCH net-next v4 3/6] virtio_net: Add a lock for the command VQ.

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: <ad9f7b83e48cfd7f1463d8c728061c30a4509076.camel@redhat.com>
Date: Thu, 18 Apr 2024 12:56:47 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Heng Qi <hengqi@...ux.alibaba.com>, Jason Wang <jasowang@...hat.com>, 
	Daniel Jurgens <danielj@...dia.com>
Cc: netdev@...r.kernel.org, mst@...hat.com, xuanzhuo@...ux.alibaba.com, 
	virtualization@...ts.linux.dev, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, jiri@...dia.com
Subject: Re: [PATCH net-next v4 3/6] virtio_net: Add a lock for the command
 VQ.

On Thu, 2024-04-18 at 15:36 +0800, Heng Qi wrote:
> 
> 在 2024/4/18 下午2:42, Jason Wang 写道:
> > On Wed, Apr 17, 2024 at 3:31 AM Daniel Jurgens <danielj@...dia.com> wrote:
> > > The command VQ will no longer be protected by the RTNL lock. Use a
> > > spinlock to protect the control buffer header and the VQ.
> > > 
> > > Signed-off-by: Daniel Jurgens <danielj@...dia.com>
> > > Reviewed-by: Jiri Pirko <jiri@...dia.com>
> > > ---
> > >   drivers/net/virtio_net.c | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 0ee192b45e1e..d02f83a919a7 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -282,6 +282,7 @@ struct virtnet_info {
> > > 
> > >          /* Has control virtqueue */
> > >          bool has_cvq;
> > > +       spinlock_t cvq_lock;
> > Spinlock is instead of mutex which is problematic as there's no
> > guarantee on when the driver will get a reply. And it became even more
> > serious after 0d197a147164 ("virtio-net: add cond_resched() to the
> > command waiting loop").
> > 
> > Any reason we can't use mutex?
> 
> Hi Jason,
> 
> I made a patch set to enable ctrlq's irq on top of this patch set, which 
> removes cond_resched().
> 
> But I need a little time to test, this is close to fast. So could the 
> topic about cond_resched +
> spin lock or mutex lock be wait?

The big problem is that until the cond_resched() is there, replacing
the mutex with a spinlock can/will lead to scheduling while atomic
splats. We can't intentionally introduce such scenario.

Side note: the compiler apparently does not like guard() construct,
leading to new warning, here and in later patches. I'm unsure if the
code simplification is worthy.

Cheers,

Paolo