Re: [PATCH RFC v2 3/6] coredump: support AF_UNIX sockets

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Christian Brauner <brauner@kernel.org>
Date: Sat, 3 May 2025 07:17:10 +0200
> On Fri, May 02, 2025 at 10:23:44PM +0200, Jann Horn wrote:
> > On Fri, May 2, 2025 at 10:11 PM Christian Brauner <brauner@kernel.org> wrote:
> > > On Fri, May 02, 2025 at 04:04:32PM +0200, Jann Horn wrote:
> > > > On Fri, May 2, 2025 at 2:42 PM Christian Brauner <brauner@kernel.org> wrote:
> > > > > diff --git a/fs/coredump.c b/fs/coredump.c
> > > > [...]
> > > > > @@ -801,6 +841,73 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> > > > >                 }
> > > > >                 break;
> > > > >         }
> > > > > +       case COREDUMP_SOCK: {
> > > > > +               struct file *file __free(fput) = NULL;
> > > > > +#ifdef CONFIG_UNIX
> > > > > +               ssize_t addr_size;
> > > > > +               struct sockaddr_un unix_addr = {
> > > > > +                       .sun_family = AF_UNIX,
> > > > > +               };
> > > > > +               struct sockaddr_storage *addr;
> > > > > +
> > > > > +               /*
> > > > > +                * TODO: We need to really support core_pipe_limit to
> > > > > +                * prevent the task from being reaped before userspace
> > > > > +                * had a chance to look at /proc/<pid>.
> > > > > +                *
> > > > > +                * I need help from the networking people (or maybe Oleg
> > > > > +                * also knows?) how to do this.
> > > > > +                *
> > > > > +                * IOW, we need to wait for the other side to shutdown
> > > > > +                * the socket/terminate the connection.
> > > > > +                *
> > > > > +                * We could just read but then userspace could sent us
> > > > > +                * SCM_RIGHTS and we just shouldn't need to deal with
> > > > > +                * any of that.
> > > > > +                */
> > > >
> > > > I don't think userspace can send you SCM_RIGHTS if you don't do a
> > > > recvmsg() with a control data buffer?
> > >
> > > Oh hm, then maybe just a regular read at the end would work. As soon as
> > > userspace send us anything or we get a close event we just disconnect.
> > >
> > > But btw, I think we really need a recvmsg() flag that allows a receiver
> > > to refuse SCM_RIGHTS/file descriptors from being sent to it. IIRC, right
> > > now this is a real issue that systemd works around by always calling its
> > > cmsg_close_all() helper after each recvmsg() to ensure that no one sent
> > > it file descriptors it didn't want. The problem there is that someone
> > > could have sent it an fd to a hanging NFS server or something and then
> > > it would hang in close() even though it never even wanted any file
> > > descriptors in the first place.
> > 
> > Would a recvmsg() flag really solve that aspect of NFS hangs? By the
> > time you read from the socket, the file is already attached to an SKB
> > queued up on the socket, and cleaning up the file is your task's
> > responsibility either way (which will either be done by the kernel for
> > you if you don't read it into a control message, or by userspace if it
> > was handed off through a control message).

Right.  recvmsg() is too late.  Once sendmsg() is done, the last
fput() responsibility could fall on the receiver.

Btw, I was able to implement the cmsg_close_all() equivalent at
sendmsg() with BPF LSM to completely remove the issue.

I will send a series shortly and hope you like it :)


> > The process that sent the
> > file to you might already be gone, it can't be on the hook for
> > cleaning up the file anymore.
> 
> Hm, I guess the unix_gc() runs in task context? I had thought that it
> might take care of that.

Note that unix_gc() is a garbage collector only for AF_UNIX fds
that have circular dependency:

  1) AF_UNIX sk1 sends its fd to itself

  2) AF_UNIX sk1 sends its fd to AF_UNIX sk2 and
     AF_UNIX sk2 sends its fd to AF_UNIX sk1

In these examples, file refcnts remain even after close() by all
users of fds.

So, the GC is not a mechanism to deligate fput() for fds sent
by SCM_RIGHTS.