Re: [PATCH RESEND] Fix race in process_vm_rw_core

[Christopher Yeoh]

On Tue, 31 Jan 2012 22:10:13 -0800
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Tue, Jan 31, 2012 at 9:53 PM, Christopher Yeoh <cyeoh@xxxxxxxxxxx>
> wrote:
> > + Â Â Â mm = mm_access(task, PTRACE_MODE_ATTACH);
> > + Â Â Â if (!mm || IS_ERR(mm)) {
> > + Â Â Â Â Â Â Â if (!mm)
> > + Â Â Â Â Â Â Â Â Â Â Â rc = -EINVAL;
> > + Â Â Â Â Â Â Â else
> > + Â Â Â Â Â Â Â Â Â Â Â rc = -EPERM;
> > Â Â Â Â Â Â Â Âgoto put_task_struct;
> 
> Btw, do you really want to throw away the error code?
> 
> IOW, maybe it should be
> 
>    rc = IS_ERR(mm) ? PTR_ERR(mm) : -EINVAL;
> 
> or something? Instead of forcing the EPERM? And the -EINVAL might be
> better off as an ESRCH? I dunno.

Yea, that probably makes more sense.

> Right now you turn all errors into EPERM, whether they were really
> about permission problems or not. And that just makes be a bit
> nervous. I wonder if we wouldn't be better off just returning EACCES
> (and any possible future problem) than try so hard to always return
> EPERM?
> 
> I dunno. I don't have any really *strong* opinion and I see why you do
> it, but my gut feel is still that the error number change really does
> seem a bit arbitrary.

I'm not super attached to returning EPERM instead of EACCES though I do
think it would look at bit odd from a user of the syscall view. Is it
too ugly to do:

    rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
    if (rc == -EACCES)
            rc = -EPERM;

That way we avoid the problem of overwriting EINTR and if there are
changes in the future which return different error codes we won't
override those.

If you think it is too ugly then I'll give in and just return EACESS.
Just should to get it settled before too many people start using the
syscalls.

Chris
-- 
cyeoh@xxxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/