linux-kernel - Re: uio regression in 4.18-rc1 due to "uio: Prevent device destruction while fds are open"

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] [day] [month] [year] [list]

Message-ID: <c835824b-901e-3b27-ae32-1ed2a927892a@alliedtelesis.co.nz>
Date:   Wed, 20 Jun 2018 22:00:28 +0000
From:   Hamish Martin <Hamish.Martin@...iedtelesis.co.nz>
To:     Mike Christie <mchristi@...hat.com>,
        Chris Packham <Chris.Packham@...iedtelesis.co.nz>,
        Greg KH <gregkh@...uxfoundation.org>,
        Linux Kernel List <linux-kernel@...r.kernel.org>
Subject: Re: uio regression in 4.18-rc1 due to "uio: Prevent device
 destruction while fds are open"

Hi Mike,

Thanks for reporting this problem with my patch.

To be honest I'm possibly out of my depth here. I gather doing things 
like sleeping and memory allocation are a big no-no when under spin_lock 
and I take your point that it is not realistic to expect all the 
callouts from the various uio_fops to know or respect that they might be 
under a spin_lock.

I'd take your advise on whether altering the type of locking used around 
accesses to the 'info' member of struct uio_device might be a path 
forward here or is likely to lead to similar or worse issues. For 
example could we change the spin_lock to something more heavyweight like 
a mutex?

My idea of completing Brian Russell's work by introducing this locking 
construct is also possibly the wrong approach. It seems nice that we can 
make the uio core cope with the disappearance of the uio_info memory 
that the info member points to, but I'm beginning to think that we 
should consider a more fundamental change to allow the uio core to 
control the life cycle of that uio_info structure. This spin_lock idea 
seemed like a clean way of avoiding that, but perhaps we just need to 
lance that boil in a more wide-ranging fashion and accept the 
consequences such as breaking out-of-tree drivers. Such a wide ranging 
change would be better done out of the release period and reverting this 
change would be the best response to this regression for 4.18.

If anyone else wants to weigh in here please do so.

Thanks,
Hamish M.

On 06/21/2018 04:06 AM, Mike Christie wrote:
> Hi Hamish,
>
> I am hitting a regression with your patch:
>
> commit a93e7b331568227500186a465fee3c2cb5dffd1f
> Author: Hamish Martin <hamish.martin@...iedtelesis.co.nz>
> Date:   Mon May 14 13:32:23 2018 +1200
>
>      uio: Prevent device destruction while fds are open
>
> The problem is the addition of spin_lock_irqsave in uio_write. This
> leads to hitting  uio_write -> copy_from_user -> _copy_from_user ->
> might_fault and the logs filling up with sleeping warnings.
>
> I also noticed some uio drivers allocate memory, sleep, grab mutexes
> from callouts like open() and release and uio is now doing
> spin_lock_irqsave while calling them.
>
> Note target_core_user's irqcontrol looks buggy and was just doing more
> work than it should in that callout. I can fix that driver.