Date: Sun, 15 Feb 2015 14:44:44 +0800
From: Fam Zheng <famz@redhat.com>
To: Omar Sandoval <osandov@osandov.com>
Cc: linux-kernel@vger.kernel.org, famz@redhat.com
Subject: Re: [PATCH RFC v3 0/7] epoll: Introduce new syscalls,
 epoll_ctl_batch and epoll_pwait1
Message-ID: <20150215064444.GB6093@fam-t430.nay.redhat.com>
References: <1423818243-15410-1-git-send-email-famz@redhat.com>
 <20150213095357.GA20668@mew>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150213095357.GA20668@mew>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3297
Lines: 70

On Fri, 02/13 01:53, Omar Sandoval wrote:
<snip>
> > Discussion
> > ==========
> > 
> > [Note: This is the question part regarding the interface contract of
> > epoll_ctl_batch.  If you don't have the context of what is epoll_ctl_batch yet,
> > please skip this part and probably start with the man page style documentation.
> > You can resume to this section later.]
> > 
> > [Thanks to Omar Sandoval <osandov@osandov.com>, who pointed out this in
> > reviewing v1]
> > 
> > We try to report status for each command in epoll_ctl_batch, by writting to
> > user provided command array (pointed to cmds). The tricky thing in the
> > implementation is that, copying the results back to userspace comes last, after
> > the commands are executed. At this point, if the copy_to_user fails, the
> > effects are done and no return - or if we add some mechanism to revert it, the
> > code will be too complicated and slow.
> > 
> > In above corner case, the return value of epoll_ctl_batch is smaller than
> > ncmds, which assures our caller that last N commands failed, where N = ncmds -
> > ret.  But they'll also find that cmd.result is not changed to error code.
> > 
> > I suppose this is not a big deal, because 1) it should happen very rarely. 2)
> > user does know the actual number of commands that succeed.
> > 
> > So, do we leave it as is? Or is there any way to improve?
> > 
> > One tiny improvement (not a complete fix) in my mind is a testing copy_to_user
> > before we execute the commands. If it succeeds, it's even less likely the last
> > copy_to_user could fail, so that we can even probably assert it won't. The
> > testing 'copy_to_user(cmds, &kcmds, ...)' will not hurt functionality if do it
> > right after 'copy_from_user(&kcmds, cmds, ...)'. But I'm not sure about the
> > performance impact, especially when @ncmds is big.
> > 
> I don't think this is the right thing to do, since, for example, another
> thread could unmap the memory region holding buffer between the "check"
> copy_to_user and the actual one.
> 
> The two alternatives that I see are:
> 
> 1. If copy_to_user fails, ignore it and return the number of commands
> that succeeded (i.e., what the code in your patch does).
> 2. If copy_to_user fails, return -EFAULT, regardless of how many
> commands succeeded.
> 
> The problem with 1 is that it could potentially mask bugs in a user
> program. You could imagine a buggy program that passes a read-only
> buffer to epoll_ctl_batch and never finds out about it because they
> don't get the error. (Then, when there's a real error in one of the
> epoll_ctl_cmds, but .result is 0, someone will be very confused.)
> 
> So I think 2 is the better option. Sure, the user will have no idea how
> many commands were executed, but when would EFAULT on this system call
> be part of normal operation, anyways? You'll find the memory bug, fix
> it, and rest easy knowing that nothing is silently failing behind your
> back.
> 

OK, I agree with you here. I'm going to change this to -EFAULT in the next
revision.

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