Hi Hamish,
I am hitting a regression with your patch:
commit a93e7b331568227500186a465fee3c2cb5dffd1f
Author: Hamish Martin <[email protected]>
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.
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 <[email protected]>
> 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.