[PATCH] bio: return EINTR if copying to user space got interrupted

[PATCH] bio: return EINTR if copying to user space got interrupted

[Hannes Reinecke]

Commit 35dc248383bbab0a7203fca4d722875bc81ef091 introduced a check for
current->mm to see if we have a user space context and only copies data
if we do. Now if an IO gets interrupted by a signal data isn't copied
into user space any more (as we don't have a user space context) but
user space isn't notified about it.

This patch modifies the behaviour to return -EINTR from bio_uncopy_user()
to notify userland that a signal has interrupted the syscall, otherwise
it could lead to a situation where the caller may get a buffer with
no data returned.

This can be reproduced by issuing SG_IO ioctl()s in one thread while
constantly sending signals to it.

Fixes: 35dc248 [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: stable@vger.kernel.org # v.3.11+
---
 block/bio.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index dbabd48..24e5b69 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1090,9 +1090,12 @@ int bio_uncopy_user(struct bio *bio)
 	if (!bio_flagged(bio, BIO_NULL_MAPPED)) {
 		/*
 		 * if we're in a workqueue, the request is orphaned, so
-		 * don't copy into a random user address space, just free.
+		 * don't copy into a random user address space, just free
+		 * and return -EINTR so user space doesn't expect any data.
 		 */
-		if (current->mm && bio_data_dir(bio) == READ)
+		if (!current->mm)
+			ret = -EINTR;
+		else if (bio_data_dir(bio) == READ)
 			ret = bio_copy_to_iter(bio, bmd->iter);
 		if (bmd->is_our_pages)
 			bio_free_pages(bio);
-- 
1.8.5.6

Re: [PATCH] bio: return EINTR if copying to user space got interrupted

[Jens Axboe]

On 02/12/2016 01:39 AM, Hannes Reinecke wrote:
> Commit 35dc248383bbab0a7203fca4d722875bc81ef091 introduced a check for
> current->mm to see if we have a user space context and only copies data
> if we do. Now if an IO gets interrupted by a signal data isn't copied
> into user space any more (as we don't have a user space context) but
> user space isn't notified about it.
>
> This patch modifies the behaviour to return -EINTR from bio_uncopy_user()
> to notify userland that a signal has interrupted the syscall, otherwise
> it could lead to a situation where the caller may get a buffer with
> no data returned.
>
> This can be reproduced by issuing SG_IO ioctl()s in one thread while
> constantly sending signals to it.

Good catch, fix looks good to me. Applied for 4.5.

-- 
Jens Axboe

Re: [PATCH] bio: return EINTR if copying to user space got interrupted

[Douglas Gilbert]

On 16-02-12 03:39 AM, Hannes Reinecke wrote:
> Commit 35dc248383bbab0a7203fca4d722875bc81ef091 introduced a check for
> current->mm to see if we have a user space context and only copies data
> if we do. Now if an IO gets interrupted by a signal data isn't copied
> into user space any more (as we don't have a user space context) but
> user space isn't notified about it.
>
> This patch modifies the behaviour to return -EINTR from bio_uncopy_user()
> to notify userland that a signal has interrupted the syscall, otherwise
> it could lead to a situation where the caller may get a buffer with
> no data returned.

Interesting, the "f091" commit has been in the kernel since 2013
hence your reference to v.3.11 . I always had the feeling that
handling signals that interrupted SG_IO calls was skating on thin
ice. Hence in ddpt (but not sg_dd nor sgp_dd) the code masks out
all signals (that it can) during the SG_IO calls then opens a
signal window briefly after a SG_IO ioctl has finished and before
the next one starts. This approach used by ddpt is borrowed from
dd (in coreutils) which masks signals during its read() and
write() calls.

Any idea how accurate resid is in this scenario?

Doug Gilbert

> This can be reproduced by issuing SG_IO ioctl()s in one thread while
> constantly sending signals to it.
>
> Fixes: 35dc248 [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Cc: stable@vger.kernel.org # v.3.11+
> ---
>   block/bio.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index dbabd48..24e5b69 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1090,9 +1090,12 @@ int bio_uncopy_user(struct bio *bio)
>   	if (!bio_flagged(bio, BIO_NULL_MAPPED)) {
>   		/*
>   		 * if we're in a workqueue, the request is orphaned, so
> -		 * don't copy into a random user address space, just free.
> +		 * don't copy into a random user address space, just free
> +		 * and return -EINTR so user space doesn't expect any data.
>   		 */
> -		if (current->mm && bio_data_dir(bio) == READ)
> +		if (!current->mm)
> +			ret = -EINTR;
> +		else if (bio_data_dir(bio) == READ)
>   			ret = bio_copy_to_iter(bio, bmd->iter);
>   		if (bmd->is_our_pages)
>   			bio_free_pages(bio);
>

Re: [PATCH] bio: return EINTR if copying to user space got interrupted

[Hannes Reinecke]

On 02/12/2016 05:05 PM, Douglas Gilbert wrote:
> On 16-02-12 03:39 AM, Hannes Reinecke wrote:
>> Commit 35dc248383bbab0a7203fca4d722875bc81ef091 introduced a check for
>> current->mm to see if we have a user space context and only copies data
>> if we do. Now if an IO gets interrupted by a signal data isn't copied
>> into user space any more (as we don't have a user space context) but
>> user space isn't notified about it.
>>
>> This patch modifies the behaviour to return -EINTR from bio_uncopy_user()
>> to notify userland that a signal has interrupted the syscall, otherwise
>> it could lead to a situation where the caller may get a buffer with
>> no data returned.
>
> Interesting, the "f091" commit has been in the kernel since 2013
> hence your reference to v.3.11 . I always had the feeling that
> handling signals that interrupted SG_IO calls was skating on thin
> ice.

Yeah; that bug was really annoying, as occasionally receiving no data 
whatsoever without any indication makes it really, really hard to debug.
Kudos go to Johannes and Ewan for pointing to the offending function.

> Hence in ddpt (but not sg_dd nor sgp_dd) the code masks out
> all signals (that it can) during the SG_IO calls then opens a
> signal window briefly after a SG_IO ioctl has finished and before
> the next one starts. This approach used by ddpt is borrowed from
> dd (in coreutils) which masks signals during its read() and
> write() calls.
>
> Any idea how accurate resid is in this scenario?
>
Bah. F*sk knows.
That takes far more POSIX knowledge than I have.
I would suspect that by masking out signals they'll be marked as pending 
for the process, and will be delivered once you unmask them.
Or dropped, depending.
In either case, once they are blocked out the kernel part shouldn't 
receive a signal, and hence we should be able to receive the data properly.
But as noted I'm not a POSIX expert.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH] bio: return EINTR if copying to user space got interrupted

[Ewan Milne]

On Fri, 2016-02-12 at 09:39 +0100, Hannes Reinecke wrote:
> Commit 35dc248383bbab0a7203fca4d722875bc81ef091 introduced a check for
> current->mm to see if we have a user space context and only copies data
> if we do. Now if an IO gets interrupted by a signal data isn't copied
> into user space any more (as we don't have a user space context) but
> user space isn't notified about it.
> 
> This patch modifies the behaviour to return -EINTR from bio_uncopy_user()
> to notify userland that a signal has interrupted the syscall, otherwise
> it could lead to a situation where the caller may get a buffer with
> no data returned.
> 
> This can be reproduced by issuing SG_IO ioctl()s in one thread while
> constantly sending signals to it.

Well, this is definitely an improvement, since it now indicates to
the user that there was a problem instead of silently returning with
no data and no error, but it doesn't completely fix the issue.

For one thing, if the SG_IO is performed to a sequential media device,
the I/O is actually performed, and the position changes, it is just the
data that is not copied back.  So e.g. a user program making calls to
read from a tape device that gets signals and retrying on -EINTR will
skip blocks.  There is a similar problem already with SG_IO and
-ERESTARTSYS.  I believe that that only way around this is to mask the
signals.  (This is also problem with non-idempotent commands, such as
COMPARE AND WRITE.)

I should mention that the patch "sg: Fix user memory corruption when
SG_IO is interrupted by a signal" broke a 3rd-party kernel module that
happened to use SG_IO to pass *kernel* addresses for I/O.  Which worked
for them until the current->mm test was added.  (I don't know why they
didn't just put I/O on the request queue like everyone else, though.)

This patch should go in, though.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

> 
> Fixes: 35dc248 [SCSI] sg: Fix user memory corruption when SG_IO is interrupted by a signal
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Cc: stable@vger.kernel.org # v.3.11+
> ---
>  block/bio.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index dbabd48..24e5b69 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1090,9 +1090,12 @@ int bio_uncopy_user(struct bio *bio)
>  	if (!bio_flagged(bio, BIO_NULL_MAPPED)) {
>  		/*
>  		 * if we're in a workqueue, the request is orphaned, so
> -		 * don't copy into a random user address space, just free.
> +		 * don't copy into a random user address space, just free
> +		 * and return -EINTR so user space doesn't expect any data.
>  		 */
> -		if (current->mm && bio_data_dir(bio) == READ)
> +		if (!current->mm)
> +			ret = -EINTR;
> +		else if (bio_data_dir(bio) == READ)
>  			ret = bio_copy_to_iter(bio, bmd->iter);
>  		if (bmd->is_our_pages)
>  			bio_free_pages(bio);