On 2019/08/25 18:59, Ingo Molnar wrote:
>> @@ -132,8 +132,10 @@ static ssize_t read_mem(struct file *file, char __user
>> *buf,
>> #endif
>>
>> bounce = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> - if (!bounce)
>> - return -ENOMEM;
>> + if (!bounce) {
>> + err = -ENOMEM;
>> + goto failed;
>> + }
>
> Yeah, so while I agree with the more consistent handling of partial
> reads, I'd suggest the following changes:
>
> - Please don't use this 4-line error handling variant, use the old short
> 2-line pattern instead. There's no real reason to keep 'err' as a
> flag, the 'failed' branch will know that 'err' is the error return if
> there's been no progress.
The caller might guarantee count > 0, but for robustness, I decided to
choose 4-line error handling here because I merged the normal and the
failure path control flow; read will remain 0 if count == 0, and thus
err should remain 0.
>
> - We should probably separate out a third 'fatal error' variant: for
> example if copying to user-space generates a page fault, then we
> clearly should not pretend that all is fine and return a short read
> even if we made some progress, a -EFAULT is more informative, as we
> might have corrupted (overran) some user buffer on the failed copy as
> well, and ran off the end into the first unmapped user area.
Is it possible that copy_from_user() corrupts user buffer in a way that
userspace
cannot retry when kernel responded with "there was a short write"? It seems that
these functions are difficult to return appropriate errors...
>
> - As for the patch series maybe it might make sense to separate the
> fixes from the semantic changes, in case there's any breakage. I.e.
> first fix the bug minimally, then add the other changes in a separate
> commit. If any of them causes problems with applications we'll have a
> more precise bisection result.
Yes. I think for now we should just make these functions killable. Then, we
can try changing return code for partial read/write. If no breakage report,
we can make these functions interruptible.
>
> - Likewise, the changing of the write side interruptability of /dev/mem
> should probably be a separate patch as well.
>
> I can factor out such a series if you don't have the time, but feel free
> to do it yourself, this is your bug report and your patch. :)
You can do it. It's a syzbot's bug report. I just forwarded it. ;-)
