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

Message-ID: <877f0mb2b6.fsf@notabene.neil.brown.name>
Date:   Fri, 09 Jun 2017 07:24:29 +1000
From:   NeilBrown <neilb@...e.com>
To:     Mikulas Patocka <mpatocka@...hat.com>, Shaohua Li <shli@...nel.org>
Cc:     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, 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.

Thanks,
NeilBrown


>
>> Thanks,
>> Shaohua
>
> Mikulas

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