linux-kernel - Re: [PATCH] md: don't use flush_signals in userspace processes

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]

Date:   Fri, 09 Jun 2017 11:49:45 +1000
From:   NeilBrown <neilb@...e.com>
To:     Shaohua Li <shli@...nel.org>
Cc:     Mikulas Patocka <mpatocka@...hat.com>, linux-raid@...r.kernel.org,
        linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] md: don't use flush_signals in userspace processes

On Thu, Jun 08 2017, Shaohua Li wrote:

> On Fri, Jun 09, 2017 at 07:24:29AM +1000, Neil Brown wrote:
>> On Thu, Jun 08 2017, Mikulas Patocka wrote:
>> 
>> > On Thu, 8 Jun 2017, Shaohua Li wrote:
>> >
>> >> On Thu, Jun 08, 2017 at 04:59:03PM +1000, Neil Brown wrote:
>> >> > On Wed, Jun 07 2017, Mikulas Patocka wrote:
>> >> > 
>> >> > > The function flush_signals clears all pending signals for the process. It
>> >> > > may be used by kernel threads when we need to prepare a kernel thread for
>> >> > > responding to signals. However using this function for an userspaces
>> >> > > processes is incorrect - clearing signals without the program expecting it
>> >> > > can cause misbehavior.
>> >> > >
>> >> > > The raid1 and raid5 code uses flush_signals in its request routine because
>> >> > > it wants to prepare for an interruptible wait. This patch drops
>> >> > > flush_signals and uses sigprocmask instead to block all signals (including
>> >> > > SIGKILL) around the schedule() call. The signals are not lost, but the
>> >> > > schedule() call won't respond to them.
>> >> > >
>> >> > > Signed-off-by: Mikulas Patocka <mpatocka@...hat.com>
>> >> > > Cc: stable@...r.kernel.org
>> >> > 
>> >> > Thanks for catching that!
>> >> > 
>> >> > Acked-by: NeilBrown <neilb@...e.com>
>> >> 
>> >> Applied, thanks!
>> >> 
>> >> Neil,
>> >> Not about the patch itself. I had question about that part of code. Dropped
>> >> others since this is raid related. I didn't get the point why it's a
>> >> TASK_INTERRUPTIBLE sleep. It seems suggesting the thread will bail out if a
>> >> signal is sent. But I didn't see we check the signal and exit the loop. What's
>> >> the correct behavior here? Since the suspend range is controlled by userspace,
>> >
>> > As I understand the code - the purpose is to have an UNINTERRUPTIBLE sleep 
>> > that isn't accounted in load average and that doesn't trigger the hung 
>> > task warning.
>> 
>> Exactly my reason - yes.
>> 
>> >
>> > There should really be something like TASK_UNINTERRUPTIBLE_LONG for this 
>> > purpose.
>> 
>> That would be nice.
>> 
>> >
>> >> I think the correct behavior is if user kills the thread, we exit the loop. So
>> >> it seems like to be we check if there is fatal signal pending, exit the loop,
>> >> and return IO error. Not sure if we should return IO error though.
>> >
>> > No, this is not correct - if we report an I/O error for the affected bio, 
>> > it could corrupt filesystem or confuse other device mapper targets that 
>> > could be on the top of MD. It is not right to corrupt filesystem if the 
>> > user kills a process.
>> 
>> Yes, we are too deep to even return something like ERESTARTSYS.
>> Blocking is the only option.
>
> My concern is if the app controlling the suspend range dies, other threads
> will block in the kernel side forever. We can't even force kill them.  This
> is an unfortunate behavior. Would adding a timeout here make sense? The app
> controlling the suspend range looks part of the disk firmware now. If the
> firmware doesn't respond, returning IO timeout is normal.

Yes, this happens.
You can write to /sys/block/mdXX/md/suspend_lo to unblock it, but most
people don't know this.

A timeout might be appropriate, but I would want it to be quite a long
one.  Several minutes at least...
Though if I found I wanted to do some careful raid6repair surgery on an
array, and so suspending the problematic region and started work, I
might get annoyed if I/O started getting errors, instead of just waiting
like I asked.... but maybe that is a rather far-fetched scenario.

Thanks,
NeilBrown


>
> Thanks,
> Shaohua

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)