epoll may leak events on dup

epoll may leak events on dup

[Sargun Dhillon]

I discovered an interesting behaviour in epoll today. If I register the same 
file twice, under two different file descriptor numbers, and then I close one of 
the two file descriptors, epoll "leaks" the first event. This is fine, because 
one would think I could just go ahead and remove the event, but alas, that isn't 
the case. Some example python code follows to show the issue at hand.

I'm not sure if this is really considered a "bug" or just "interesting epoll
behaviour", but in my opinion this is kind of a bug, especially because leaks
may happen by accident -- especially if files are not immediately freed.

I'm also not sure why epoll events are registered by file, and not just fd.
Is the expectation that you can share a single epoll amongst multiple
"users" and register different files that have the same file descriptor
number (at least for purposes other than CRIU). Maybe someone can shed
light on the behaviour.

(FWIW: On closing of all references the file, the callback finally occurs
clearing the event)

#!/usr/bin/env python3
import select
import socket
import os

n = 2
sockets = []
epoll = select.epoll()

for i in range(n):
        sockets.append(socket.socket(socket.AF_INET6, socket.SOCK_DGRAM))
        sockets[-1].bind(('::1', 0))

for i in sockets:
        epoll.register(i)

clones = [os.dup(i.fileno()) for i in sockets]

epoll.poll()
# Returns: [(4, 4), (5, 4)]

for i in clones:
        epoll.register(i)

epoll.poll()
# Returns: [(4, 4), (5, 4), (6, 4), (7, 4)]

socket0name = sockets[0].getsockname()
socket0fileno = sockets[0].fileno()
print(f'Closing {socket0fileno}')
os.close(socket0fileno)

epoll.poll()
# Returns: [(4, 4), (5, 4), (6, 4), (7, 4)]

badsocket = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)
badsocket.sendto(b'test', (socket0name[0], socket0name[1]))

epoll.poll()
# Returns [(4, 5), (5, 4), (6, 5), (7, 4)]
# Indicating fd 4, and 6 have events.

# Oh whoops, I forgot to unregister 4...
epoll.unregister(4)

# Returns:
# Traceback (most recent call last):
#   File "<stdin>", line 1, in <module>
# FileNotFoundError: [Errno 2] No such file or directory


print(open(f'/proc/{os.getpid()}/fdinfo/{epoll.fileno()}').read())
# Returns:
# pos:  0
# flags:        02000002
# mnt_id:       15
# ino:  10994
# tfd:        5 events:       1f data:                5  pos:0 ino:30ffc5 sdev:8
# tfd:        7 events:       1f data:                7  pos:0 ino:30ffc5 sdev:8
# tfd:        4 events:       1f data:                4  pos:0 ino:30ffc4 sdev:8
# tfd:        6 events:       1f data:                6  pos:0 ino:30ffc4 sdev:8

Re: epoll may leak events on dup

[Eric Wong]

Sargun Dhillon <sargun@sargun.me> wrote:
> I discovered an interesting behaviour in epoll today. If I register the same 
> file twice, under two different file descriptor numbers, and then I close one of 
> the two file descriptors, epoll "leaks" the first event. This is fine, because 
> one would think I could just go ahead and remove the event, but alas, that isn't 
> the case. Some example python code follows to show the issue at hand.
>
> I'm not sure if this is really considered a "bug" or just "interesting epoll
> behaviour", but in my opinion this is kind of a bug, especially because leaks
> may happen by accident -- especially if files are not immediately freed.

"Interesting epoll behavior" combined with a quirk with the
Python wrapper for epoll.  It passes the FD as epoll_event.data
(.data could also be any void *ptr, a u64, or u32).

Not knowing Python myself (but knowing Ruby and Perl5 well); I
assume Python developers chose the safest route in passing an
integer FD for .data.  Passing a pointer to an arbitrary
Perl/Ruby object would cause tricky lifetime issues with the
automatic memory management of those languages; I expect Python
would have the same problem.

> I'm also not sure why epoll events are registered by file, and not just fd.
> Is the expectation that you can share a single epoll amongst multiple
> "users" and register different files that have the same file descriptor

No, the other way around.  Different FDs for the same file.

Having registration keyed by [file+fd] allows users to pass
different pointers for different events to the same file;
which could have its uses.

Registering by FD alone isn't enough; since the epoll FD itself
can be shared across fork (which is of limited usefulness[1]).
Originaly iterations of epoll were keyed only by the file;
with the FD being added later.

> number (at least for purposes other than CRIU). Maybe someone can shed
> light on the behaviour.

CRIU?  Checkpoint/Restore In Userspace?


[1] In contrast, kqueue has a unique close-on-fork behavior
    which greatly simplifies usage from C code (but less so
    for high-level runtimes which auto-close FDs).

Re: epoll may leak events on dup

[Sargun Dhillon]

On Sun, Oct 31, 2021 at 07:39:23AM +0000, Eric Wong wrote:
> Sargun Dhillon <sargun@sargun.me> wrote:
> > I discovered an interesting behaviour in epoll today. If I register the same 
> > file twice, under two different file descriptor numbers, and then I close one of 
> > the two file descriptors, epoll "leaks" the first event. This is fine, because 
> > one would think I could just go ahead and remove the event, but alas, that isn't 
> > the case. Some example python code follows to show the issue at hand.
> >
> > I'm not sure if this is really considered a "bug" or just "interesting epoll
> > behaviour", but in my opinion this is kind of a bug, especially because leaks
> > may happen by accident -- especially if files are not immediately freed.
> 
> "Interesting epoll behavior" combined with a quirk with the
> Python wrapper for epoll.  It passes the FD as epoll_event.data
> (.data could also be any void *ptr, a u64, or u32).
> 
> Not knowing Python myself (but knowing Ruby and Perl5 well); I
> assume Python developers chose the safest route in passing an
> integer FD for .data.  Passing a pointer to an arbitrary
> Perl/Ruby object would cause tricky lifetime issues with the
> automatic memory management of those languages; I expect Python
> would have the same problem.
> 

Python was just chosen as a way to have some inline code to explain the problem. 
Python has a bunch of libraries that will properly manage epoll under the hood, 
but the point was to describe the "leak" behaviour, where you cannot (easily) 
free up the registered epoll_event.

It was shorter inline code than C. :).

> > I'm also not sure why epoll events are registered by file, and not just fd.
> > Is the expectation that you can share a single epoll amongst multiple
> > "users" and register different files that have the same file descriptor
> 
> No, the other way around.  Different FDs for the same file.
> 
> Having registration keyed by [file+fd] allows users to pass
> different pointers for different events to the same file;
> which could have its uses.
> 
> Registering by FD alone isn't enough; since the epoll FD itself
> can be shared across fork (which is of limited usefulness[1]).
> Originaly iterations of epoll were keyed only by the file;
> with the FD being added later.
> 
> > number (at least for purposes other than CRIU). Maybe someone can shed
> > light on the behaviour.
> 
> CRIU?  Checkpoint/Restore In Userspace?
> 
Right, in CRIU, epoll is restored by manually cloning the FDs to the
right spot, and re-installing the events into epoll. This requires:
0. Getting the original epoll FD
1. Fetching / recreating the original FD
2. dup2'ing it to right spot (and avoiding overwriting the original epoll FD)
3. EPOLL_CTL_ADD'ing the FD back in.

> 
> [1] In contrast, kqueue has a unique close-on-fork behavior
>     which greatly simplifies usage from C code (but less so
>     for high-level runtimes which auto-close FDs).

Perhaps a question for epoll developers and maintains, how would you feel about 
the idea of adding a new set of commands that allow the user to add / mod / del 
by arbitrary key. For example, EPOLL_CTL_ADD_BY_KEY, EPOLL_CTL_MOD_BY_KEY, 
EPOLL_CTL_DEL_BY_KEY, and instead the fd argument would be an arbitrary key, and 
the object for add would be:


struct epoll_event_with_fd {
	uint32_t	fd;	
	epoll_data_t	data;
}

EPOLL_CTL_MOD_BY_KEY and EPOLL_CTL_DEL_BY_KEY would just treat the fd argument 
as a key. In order for this not to be horrible (IMHO), we would have to make
epoll run in a mode where only one event can be registered for a given key.

Then the rb_tree key, instead of being:
struct epoll_filefd {
	struct file *file;
	int fd;
} __packed;

would be:
struct epoll_filefd {
	struct file *file;
	union {
		int fd;
		int key;
	}
} __packed;

Perhaps this is rambly, and code / patches are required for better articulation,
but I guess the whole idea "fd is the key" for entries in epoll seems weird to
me, and I'm wondering if people would be open to changing the API at this point
in time.

Re: epoll may leak events on dup

[Eric Wong]

Sargun Dhillon <sargun@sargun.me> wrote:
> On Sun, Oct 31, 2021 at 07:39:23AM +0000, Eric Wong wrote:
> > CRIU?  Checkpoint/Restore In Userspace?
> > Sargun Dhillon <sargun@sargun.me> wrote:
> > 
> Right, in CRIU, epoll is restored by manually cloning the FDs to the
> right spot, and re-installing the events into epoll. This requires:
> 0. Getting the original epoll FD
> 1. Fetching / recreating the original FD
> 2. dup2'ing it to right spot (and avoiding overwriting the original epoll FD)
> 3. EPOLL_CTL_ADD'ing the FD back in.

OK, am I understanding it's something like:

	int tmp_fd = epoll_create1(...);
	if (tmp_fd != orig_epfd) {
		dup2(tmp_fd, orig_epfd);
		close(tmp_fd);
	}

	for (/* loop over original FDs: */) {
		tmp_fd = socket(...);
		if (tmpfd != orig_sfd) {
			dup2(tmp_fd, orig_sfd);
			close(tmp_fd);
		}
		epoll_ctl(orig_epfd, EPOLL_CTL_ADD, orig_sfd, ...);
	}

Is that close to what CRIU is doing?
In no place does tmp_fd end up in the epoll rbtree, there.
Everything is keyed w/ orig_sfd and it's underlying file.