file: Add a fdclose method

[markj]

Summary

Consider a program that creates a unix socket pair, transmits both
sockets from one to the other using an SCM_RIGHTS message, and then
closes both sockets without externalizing the message. unp_gc() is
supposed to handle cleanup, but it is only triggered by uipc_detach(),
which runs when a unix socket is destroyed. Because the two sockets are
internalized, their refcounts are positive, so uipc_detach() isn't
called.

As a result, a userspace program can create an unbounded amount of
garbage without triggering reclaim.

I propose triggering garbage collection whenever a unix socket is
close()d. To implement this, add new a fdclose file op and protocol op,
and implement them accordingly. Since mqueuefs has a hack to hook into
the file close path, convert it to use the new op as well.

Now, userspace can't create garbage without triggering reclamation.

[glebius]

sys/sys/protosw.h

125

Any better comment here?

[kib]

sys/kern/uipc_usrreq.c

912

As I understand it, there is a small wart that nothing guarantees that we execute the scheduled gc after the file reference is dropped in close. I do think that this is not worth fixing, we would not allow to much unix socket garbage to accumulate for many reasons, but I think that it is worth mentioning it in the comment.

sys/sys/file.h

157

Just in case, the commit needs MFC, and then you would use spare slot instead of adding it there.
That said, why the fo_close() method was not good enough, requiring a new one? I understand that mq wants filedesc lock, but what about unix sockets?

[markj]

sys/kern/uipc_usrreq.c

912

Yes, you are right. Maybe, instead of adding this new method, we should schedule GC whenever a unix socket is created.

sys/sys/file.h

157

It's because fo_close() is only closed once all file refs have gone away, so an internalized reference can keep the file alive. Thus, fo_close() is not closed promptly and garbage can accumulate. fo_fdclose represents the scenario where a user called close(2); of course, we may end up with multiple fo_fdclose calls for a single file. I am not sure yet whether it is a good interface. Certainly I will add some comments.

[kib]

sys/kern/uipc_usrreq.c

912

You mean 'closed', instead of 'created'? Either should work, but on create it would allow for much larger window.

I think that the new method is better if only it removes the pollution of the desc code by mq.

sys/sys/file.h

157

Right, I missed the 'last ref' part.

[markj]

sys/kern/uipc_usrreq.c

912

No, I meant created. Today, userspace can avoid GC with a loop like

int s[2];

while (1) {
    socketpair(PF_LOCAL, SOCK_DGRAM, s);
    sendfd(s[0], s[0]);
    sendfd(s[0], s[1]);
    close(s[0]);
    close(s[1]);
}

(I discovered this by accident while trying to write a regression test for a different bug.) If we call schedule_gc() from uipc_attach(), then userspace also cannot create unbounded garbage, since the socketpair() call will trigger GC. I think using fo_fdclose() is probably better in the end.