netdev - Re: [BUG] Inconsistent lock state in virtnet poll

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]

Date:   Tue, 5 May 2020 19:54:49 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     netdev@...r.kernel.org, Jason Wang <jasowang@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [BUG] Inconsistent lock state in virtnet poll

On Wed, May 06, 2020 at 12:30:52AM +0200, Thomas Gleixner wrote:
> "Michael S. Tsirkin" <mst@...hat.com> writes:
> > On Tue, May 05, 2020 at 02:08:56PM +0200, Thomas Gleixner wrote:
> >> 
> >> The following lockdep splat happens reproducibly on 5.7-rc4
> >
> >> ================================
> >> WARNING: inconsistent lock state
> >> 5.7.0-rc4+ #79 Not tainted
> >> --------------------------------
> >> inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> >> ip/356 [HC0[0]:SC1[1]:HE1:SE0] takes:
> >> f3ee4cd8 (&syncp->seq#2){+.?.}-{0:0}, at: net_rx_action+0xfb/0x390
> >> {SOFTIRQ-ON-W} state was registered at:
> >>   lock_acquire+0x82/0x300
> >>   try_fill_recv+0x39f/0x590
> >
> > Weird. Where does try_fill_recv acquire any locks?
> 
>   u64_stats_update_begin(&rq->stats.syncp);
> 
> That's a 32bit kernel which uses a seqcount for this. sequence counts
> are "lock" constructs where you need to make sure that writers are
> serialized.
> 
> Actually the problem at hand is that try_fill_recv() is called from
> fully preemptible context initialy and then from softirq context.
> 
> Obviously that's for the open() path a non issue, but lockdep does not
> know about that. OTOH, there is other code which calls that from
> non-softirq context.

Well to be more specific, try_fill_recv is either called from napi,
or from preemptible contexts with napi disabled and rtnl lock taken.

Two try_fill_recv calls in parallel would cause ring corruption
and no end of mischief, we certainly don't want that even on 64 bit
kernels.


> The hack below made it shut up. It's obvioulsy not ideal, but at least
> it let me look at the actual problem I was chasing down :)
> 
> Thanks,
> 
>         tglx

I'll post a hack based on Eric's suggestion since that is at least
0-overhead on 64 bit. It would be nice if we could teach
lockdep that it's about napi...


> 8<-----------
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1243,9 +1243,11 @@ static bool try_fill_recv(struct virtnet
>  			break;
>  	} while (rq->vq->num_free);
>  	if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
> +		local_bh_disable();
>  		u64_stats_update_begin(&rq->stats.syncp);
>  		rq->stats.kicks++;
>  		u64_stats_update_end(&rq->stats.syncp);
> +		local_bh_enable();
>  	}
>  
>  	return !oom;