Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 18 May 2020 18:10:58 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Pavel Machek <pavel@denx.de>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        syzbot+c8a8197c8852f566b9d9@syzkaller.appspotmail.com,
        syzbot+40b71e145e73f78f81ad@syzkaller.appspotmail.com,
        Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Sasha Levin <sashal@kernel.org>
Subject: Re: [PATCH 4.19 02/80] shmem: fix possible deadlocks on
 shmlock_user_lock
In-Reply-To: <20200518211330.GA25576@amd>
Message-ID: <alpine.LSU.2.11.2005181714490.1094@eggly.anvils>
References: <20200518173450.097837707@linuxfoundation.org> <20200518173450.633393924@linuxfoundation.org> <20200518211330.GA25576@amd>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Pavel,

On Mon, 18 May 2020, Pavel Machek wrote:

> Hi!
> 
> > This may not risk an actual deadlock, since shmem inodes do not take
> > part in writeback accounting, but there are several easy ways to avoid
> > it.
> 
> ...
> 
> > Take info->lock out of the chain and the possibility of deadlock or
> > lockdep warning goes away.
> 
> It is unclear to me if actual possibility of deadlock exists or not,
> but anyway:
> 
> >  	int retval = -ENOMEM;
> >  
> > -	spin_lock_irq(&info->lock);
> > +	/*
> > +	 * What serializes the accesses to info->flags?
> > +	 * ipc_lock_object() when called from shmctl_do_lock(),
> > +	 * no serialization needed when called from shm_destroy().
> > +	 */
> >  	if (lock && !(info->flags & VM_LOCKED)) {
> >  		if (!user_shm_lock(inode->i_size, user))
> >  			goto out_nomem;
> 
> Should we have READ_ONCE() here? If it is okay, are concurency
> sanitizers smart enough to realize that it is okay? Replacing warning
> with different one would not be exactly a win...

If a sanitizer comes to question this change, I don't see how a
READ_ONCE() anywhere near here (on info->flags?) is likely to be
enough to satisfy it - it would be asking for a locking scheme that
it understands (being unable to read the comment) - and might then
ask for that same locking in the numerous other places that read
info->flags (and a few that write it).  Add data_race()s all over?

(Or are you concerned about that inode->i_size, which I suppose ought
really to be i_size_read(inode) on some 32-bit configurations; though
that's of very long standing, and has never caused any concern before.)

I am not at all willing to add annotations speculatively, in case this
or that tool turns out to want help later.  So far I've not heard of
any such complaint on 5.7-rc[3456] or linux-next: but maybe this is
too soon to hear a complaint, and you feel this should not be rushed
into -stable?

This was an AUTOSEL selection, to which I have no objection, but it
isn't something we were desperate to push into -stable: so I've also
no objection if Greg shares your concern, and prefers to withdraw it.
(That choice may depend on to what extent he expects to be keeping
-stable clean against upcoming sanitizers in future.)

Hugh