(Giant disclaimer: I had not even heard of watch_queue.c until this week.
So this might be garbage, but I think at least the bug is real and I think
the analysis is correct.)
We got a bug report stating that doing this in a loop:
pipe2(pipefd, O_NOTIFICATION_PIPE);
ioctl(pipefd[0], IOC_WATCH_QUEUE_SET_SIZE, BUF_SIZE);
close(pipefd[0]);
close(pipefd[1]);
as an unprivileged user would eventually yield -EPERM. The bug actually
turned up when running
https://github.com/SELinuxProject/selinux-testsuite/tree/main/tests/watchkey
Analysis:
The -EPERM happens because the too_many_pipe_buffers_soft() test in
watch_queue_set_size() fails. That fails because user->pipe_bufs has
underflowed. user->pipe_bufs has underflowed because (abbreviated) ...
sys_pipe2
__do_pipe_flags
create_pipe_files
get_pipe_inode
alloc_pipe_info
pipe_bufs = PIPE_DEF_BUFFERS; // (16)
// charge 16 bufs to user
account_pipe_buffers(user, 0, pipe_bufs);
pipe->nr_accounted = pipe_bufs; // (16)
so now the pipe has nr_accounted set, normally to PIPE_DEF_BUFFERS (16)
Then, the ioctl:
IOC_WATCH_QUEUE_SET_SIZE
watch_queue_set_size(nr_notes)
nr_pages = nr_notes / WATCH_QUEUE_NOTES_PER_PAGE; // (8)
// reduce bufs charged to user for this pipe from 16 to 8
account_pipe_buffers(pipe->user, pipe->nr_accounted, nr_pages);
nr_notes = nr_pages * WATCH_QUEUE_NOTES_PER_PAGE;
pipe_resize_ring(pipe, roundup_pow_of_two(nr_slots == nr_notes));
if (!pipe_has_watch_queue(pipe))
pipe->nr_accounted = nr_slots;
Two things to note here:
Because pipe_has_watch_queue() is true, pipe->nr_accounted is not changed.
Also, pipe_resize_ring() resized to the number of notifications, not the
number of pages, though pipe_resize_ring() seems to expect a page count?
At this point wve hae charged 8 pages to user->pipe_bufs, but
pipe->nr_accounted is still 16. And, it seems that we have actually
allocated 8*32 pages to the pipe by virtue of calling pipe_resize_ring()
with nr_notes not nr_pages.
Finally, we close the pipes:
pipe_release
put_pipe_info
free_pipe_info
account_pipe_buffers(pipe->user, pipe->nr_accounted, 0);
This subtracts pipe->nr_accounted (16) from user->pipe_bufs, even though
per above we had only actually charged 8 to user->pipe_bufs, so it's a
net reduction. Do that in a loop, and eventually user->pipe_bufs underflows.
Maybe all that should be in the commit messages, but I'll try to reduce it for
now, any comments are appreciated.
(This does pass the handful of watch queue tests in LTP.)
thanks,
-Eric
