On Wed, Jan 22, 2020 at 5:02 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
> Also make it available outside of epoll, along with the helper that
> decides if we need to copy the passed in epoll_event.
[...]
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index cd848e8d08e2..162af749ea50 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
[...]
> -static int do_epoll_ctl(int epfd, int op, int fd, struct epoll_event *epds)
> +static inline int epoll_mutex_lock(struct mutex *mutex, int depth,
> + bool nonblock)
> +{
> + if (!nonblock) {
> + mutex_lock_nested(mutex, depth);
> + return 0;
> + }
> + if (!mutex_trylock(mutex))
> + return 0;
> + return -EAGAIN;
The documentation for mutex_trylock() says:
* Try to acquire the mutex atomically. Returns 1 if the mutex
* has been acquired successfully, and 0 on contention.
So in the success case, this evaluates to:
if (!1)
return 0;
return -EAGAIN;
which is
if (0)
return 0;
return -EAGAIN;
which is
return -EAGAIN;
I think you'll have to get rid of the negation.
> +}
> +
> +int do_epoll_ctl(int epfd, int op, int fd, struct epoll_event *epds,
> + bool nonblock)
> {
> int error;
> int full_check = 0;
> @@ -2145,13 +2152,17 @@ static int do_epoll_ctl(int epfd, int op, int fd, struct epoll_event *epds)
> * deep wakeup paths from forming in parallel through multiple
> * EPOLL_CTL_ADD operations.
> */
> - mutex_lock_nested(&ep->mtx, 0);
> + error = epoll_mutex_lock(&ep->mtx, 0, nonblock);
> + if (error)
> + goto error_tgt_fput;
> if (op == EPOLL_CTL_ADD) {
> if (!list_empty(&f.file->f_ep_links) ||
> is_file_epoll(tf.file)) {
> full_check = 1;
> mutex_unlock(&ep->mtx);
> - mutex_lock(&epmutex);
> + error = epoll_mutex_lock(&epmutex, 0, nonblock);
> + if (error)
> + goto error_tgt_fput;
When we reach the "goto", full_check==1 and epmutex is not held. But
at the jump target, this code runs:
error_tgt_fput:
if (full_check) // true
mutex_unlock(&epmutex);
So I think we're releasing a lock that we don't hold.
> if (is_file_epoll(tf.file)) {
> error = -ELOOP;
> if (ep_loop_check(ep, tf.file) != 0) {
> @@ -2161,10 +2172,17 @@ static int do_epoll_ctl(int epfd, int op, int fd, struct epoll_event *epds)
> } else
> list_add(&tf.file->f_tfile_llink,
> &tfile_check_list);
> - mutex_lock_nested(&ep->mtx, 0);
> + error = epoll_mutex_lock(&ep->mtx, 0, nonblock);
> + if (error) {
> +out_del:
> + list_del(&tf.file->f_tfile_llink);
> + goto error_tgt_fput;
> + }
> if (is_file_epoll(tf.file)) {
> tep = tf.file->private_data;
> - mutex_lock_nested(&tep->mtx, 1);
> + error = epoll_mutex_lock(&tep->mtx, 1, nonblock);
> + if (error)
> + goto out_del;
When we reach this "goto", ep->mtx is held and never dropped.
> }
> }
> }
> @@ -2233,7 +2251,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd,
> copy_from_user(&epds, event, sizeof(struct epoll_event)))
> return -EFAULT;
>
> - return do_epoll_ctl(epfd, op, fd, &epds);
> + return do_epoll_ctl(epfd, op, fd, &epds, false);
> }
