* Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp> wrote:
> On 2019/08/25 5:22, Ingo Molnar wrote:
> >> So I'd be willing to try that (and then if somebody reports a
> >> regression we can make it use "fatal_signal_pending()" instead)
> >
> > Ok, will post a changelogged patch (unless Tetsuo beats me to it?).
>
> Here is a patch. This patch also tries to fix handling of return code when
> partial read/write happened (because we should return bytes processed when
> we return due to -EINTR). But asymmetric between read function and write
> function looks messy. Maybe we should just make /dev/{mem,kmem} killable
> for now, and defer making /dev/{mem,kmem} interruptible till rewrite of
> read/write functions.
>
> drivers/char/mem.c | 89
> ++++++++++++++++++++++++++++++------------------------
> 1 file changed, 50 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index cb8e653..3c6a3c2 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -108,7 +108,7 @@ static ssize_t read_mem(struct file *file, char __user
> *buf,
> ssize_t read, sz;
> void *ptr;
> char *bounce;
> - int err;
> + int err = 0;
>
> if (p != *ppos)
> return 0;
> @@ -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.
- 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.
- 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.
- 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. :)
> @@ -180,14 +182,11 @@ static ssize_t read_mem(struct file *file, char __user
> *buf,
> count -= sz;
> read += sz;
> }
> +failed:
> kfree(bounce);
>
> *ppos += read;
> - return read;
> -
> -failed:
> - kfree(bounce);
> - return err;
> + return read ? read : err;
> }
Yeah, the merging of the normal and the failure path control flow doesn't
really help readability and makes the actual iterator more complex - I
think the old exception handling pattern was fine.
I think the same applies to the write path as well.
Thanks,
Ingo
