linux-kernel - Re: [PATCH] kernel/watch_queue: Make pipe NULL while clearing watch

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: <3558070.1658933200@warthog.procyon.org.uk>
Date:   Wed, 27 Jul 2022 15:46:40 +0100
From:   David Howells <dhowells@...hat.com>
To:     Siddh Raman Pant <code@...dh.me>
Cc:     dhowells@...hat.com, "Greg KH" <gregkh@...uxfoundation.org>,
        "Christophe JAILLET" <christophe.jaillet@...adoo.fr>,
        "Eric Dumazet" <edumazet@...gle.com>,
        "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        "linux-security-modules" <linux-security-module@...r.kernel.org>,
        "linux-kernel-mentees" 
        <linux-kernel-mentees@...ts.linuxfoundation.org>,
        "linux-kernel" <linux-kernel@...r.kernel.org>,
        "syzbot+c70d87ac1d001f29a058" 
        <syzbot+c70d87ac1d001f29a058@...kaller.appspotmail.com>
Subject: Re: [PATCH] kernel/watch_queue: Make pipe NULL while clearing watch_queue

Siddh Raman Pant <code@...dh.me> wrote:

> Greg KH <gregkh@...uxfoundation.org> wrote:

> > > -	spin_unlock_bh(&wqueue->lock);
> > >  	rcu_read_unlock();
> >
> > Also you now have a spinlock held when calling rcu_read_unlock(), are
> > you sure that's ok?

Worse, we have softirqs disabled still, which might cause problems for
rcu_read_unlock()?

> We logically should not do write operations in a read critical section, so the
> nulling of `wqueue->pipe->watch_queue` should happen after rcu_read_unlock().
> Also, since we already have a spinlock, we can use it to ensure the nulling.
> So I think it is okay.

Read/write locks are perhaps misnamed in this sense; they perhaps should be
shared/exclusive.  But, yes, we *can* do certain write operations with the
lock held - if we're careful.  Locks are required if we need to pairs of
related memory accesses; if we're only making a single non-dependent write,
then we don't necessarily need a write lock.

However, you're referring to RCU read lock.  That's a very special lock that
has to do with maintenance of persistence of objects without taking any other
lock.  The moment you drop that lock, anything you accessed under RCU protocol
rules should be considered to have evaporated.

Think of it more as a way to have a deferred destructor/deallocator.

So I would do:

+
+	/* Clearing the watch queue, so we should clean the associated pipe. */
+	if (wqueue->pipe) {
+		wqueue->pipe->watch_queue = NULL;
+		wqueue->pipe = NULL;
+	}
+
	spin_unlock_bh(&wqueue->lock);
 	rcu_read_unlock();
 }

However, since you're now changing wqueue->pipe whilst a notification is being
posted, you need a barrier in post_one_notification() to prevent the compiler
from reloading the value:

	struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe);

David