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  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:   Sun, 31 Mar 2019 15:13:47 -0400
From:   Jan Harkes <jaharkes@...cmu.edu>
To:     Waiman Long <longman@...hat.com>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Pedro Cuadra Chamorro <pcuadrac@...cmu.edu>,
        linux-kernel@...r.kernel.org
Subject: Re: fs/coda oops bisected to (925b9cd1b8) "locking/rwsem: Make owner
 store task pointer of last owni

On Sun, Mar 31, 2019 at 02:14:13PM -0400, Waiman Long wrote:
> On 03/31/2019 12:00 AM, Jan Harkes wrote:
> > On Fri, Mar 29, 2019 at 05:53:22PM +0000, Waiman Long wrote:
> >> On 03/29/2019 12:10 PM, Jan Harkes wrote:
> >>> I knew I definitely had never seen this problem with the stable kernel
> >>> on Ubuntu xenial (4.4) so I bisected between v4.4 and v5.1-rc2 and ended
> >>> up at
> >>>
> >>>     # first bad commit: [925b9cd1b89a94b7124d128c80dfc48f78a63098]
> >>>     # locking/rwsem: Make owner store task pointer of last owning reader
> >>>
> >>> When I revert this particular commit on 5.1-rc2, I am not able to
> >>> reproduce the problem anymore.
> >> Without CONFIG_DEBUG_RWSEMS, the only behavioral change of this patch is
> >> to do an unconditional write of task_structure pointer into sem->owner
> >> after acquiring the read lock in down_read(). Before this patch, it does
> >
> > I tried with just that change, but that is not at fault. It is also hard
> > to believe we have a use-after-free issue, because we are using a
> > spinlock on the inode that is held in place by the file we are releasing.
> 
> One possibility is that there is a previous reference to the memory
> currently occupied by the spinlock. If the memory location is previously
> part of a rwsem structure and someone is still using it, you may get
> memory corruption.

Ah, I hadn't even thought of that possibility. Good, it will open up
some new places to look at. Since I can open, read, write and close
files in Coda without problems and so far it only seems to be related to
closing the fd as a result of an executable process exiting.

> >     -	unsigned long val = (unsigned long)owner | RWSEM_READER_OWNED
> >     -						 | RWSEM_ANONYMOUSLY_OWNED;
> >     +	unsigned long val = RWSEM_READER_OWNED | RWSEM_ANONYMOUSLY_OWNED;
>
> The change above clears all the upper bytes of owner to 0, while the
> original code will write some non-zero values to the upper bytes.

Yes, and write locks will still write to all bits of the owner field. So
the clobber must then be caused by something trying to grab a read lock
on a rw_semaphore that has been freed, and whose memory got reused by
the inode that was allocated when starting the executable. That seems
like a specific enough thing that I hopefully will be able to find it.

> > I'll continue digging if I can find a reason why. So far I've only found
> > one place where rwsem->owner is modified while not holding a lock, but
> > changing that doesn't make a difference for my particular case.
...
> The rwsem is still locked. The above code releases the current task from
> being the lock holder of the rwsem from lockdep's perspective. Later on

Ah that explains why that change made no difference. I'm unfamiliar with
the details of the locking code and assumed it was releasing some sort
of write lock there, my bad.

Jan

Powered by blists - more mailing lists