Re: [lvc-project] [PATCH] [RFC] net: smc: fix fasync leak in smc_release()

["Antipov, Dmitriy" <Dmitriy.Antipov@softline.com>]

On Wed, 2024-02-21 at 21:09 +0800, Wen Gu wrote:

> 1. on = 1; ioctl(sock, FIOASYNC, &on), a fasync entry is added to
>     smc->sk.sk_socket->wq.fasync_list;
> 
> 2. Then fallback happend, and swapped the socket:
>     smc->clcsock->file = smc->sk.sk_socket->file;
>     smc->clcsock->file->private_data = smc->clcsock;
>     smc->clcsock->wq.fasync_list = smc->sk.sk_socket->wq.fasync_list;
>     smc->sk.sk_socket->wq.fasync_list = NULL;
> 
> 3. on = 0; ioctl(sock, FIOASYNC, &on), the fasync entry is removed
>     from smc->clcsock->wq.fasync_list,
> (Is there a race between 2 and 3 ?)

1) IIUC yes. The following sequence from smc_switch_to_fallback():

smc->clcsock->file = smc->sk.sk_socket->file;
smc->clcsock->file->private_data = smc->clcsock;

is executed with locked smc->sk.sk_socket but unlocked smc->clcsock.
This way,

struct socket *sock = filp->private_data;

in sock_fasync() introduces an undefined behavior (because an
actual value of 'private_data' is unpredictable). So there are
two possible scenarios. When

on = 1; ioctl(sock, FIOASYNC, &on);
on = 0; ioctl(sock, FIOASYNC, &on);

actually modifies fasync list of the same socket, this works as
expected. If kernel sockets behind 'sock' gets swapped between
calls to ioctl(), fasync list of the first socket has an entry
which can't be removed by the second ioctl(). 


> 4. Then close the file, __fput() calls file->f_op->fasync(-1, file, 0),
>     then sock_fasync() calls fasync_helper(fd, filp, on, &wq->fasync_list)
>     and fasync_remove_entry() removes entries in smc->clcsock->wq.fasync_list.
>     Now smc->clcsock->wq.fasync_list is empty.

2) No. In the second (bad) scenario from the above, an attempt to remove
fasync entry from smc->clcsock->wq.fasync_list always fails because
fasync entry was actually linked to smc->sk.sk_socket->wq.fasync_list.

Note sock_fasync() doesn't check the value returned from fasync_helper().
How dumb.

> 5. __fput() calls file->f_op->release(inode, file), then sock_close calls
>     __sock_release, then ops->release calls smc_release(), and __smc_release()
>     calls smc_restore_fallback_changes() to restore socket:
>     if (smc->clcsock->file) { /* non-accepted sockets have no file yet */
>          smc->clcsock->file->private_data = smc->sk.sk_socket;
>          smc->clcsock->file = NULL;
>          smc_fback_restore_callbacks(smc);
>     }

3) Yes. And it's too late because __fput() calls file->f_op->fasync(-1, ...,
0) _before_ file->f_op->release(). So even if you have sockets swapped back,
no one will take care about freeing fasync list.


> 6. Then back to __sock_release, check if sock->wq.fasync_list (that is
>     smc->sk.sk_socket->wq.fasync_list) is empty and it is empty.

4) No. It's not empty because an attempt to remove fasync entry has failed
at [2] just because it was made against the wrong socket. 


For your convenience, there is a human-readable reproducer loosely modeled
around the one generated by syzkaller. You can try it on any system running
recently enough kernel with CONFIG_SMC enabled (root is not required), and
receiving a few (or may be a lot of) "__sock_release: fasync list not empty"
messages clearly indicates an issue. NOTE: this shouldn't crash the system
and/or make it unusable, but remember that each message comes with a small
kernel memory leak.

Dmitry

#include <signal.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/ioctl.h>
#include <sys/socket.h>

int sock;

void *
loop0 (void *arg)
{
  struct msghdr msg = { 0 };

  while (1)
    {
      sock = socket (AF_SMC, SOCK_STREAM, 0);
      sendmsg (sock, &msg, MSG_FASTOPEN);
      close (sock);
    }
  return NULL;
}

void *
loop1 (void *arg)
{
  int on;

  while (1)
    {
      on = 1;
      ioctl (sock, FIOASYNC, &on);
      on = 0;
      ioctl (sock, FIOASYNC, &on);
    }

  return NULL;
}

int
main (int argc, char *argv[])
{
  pthread_t a, b;
  struct sigaction sa = { 0 };

  sa.sa_handler = SIG_IGN;
  sigaction (SIGIO, &sa, NULL);

  pthread_create (&a, NULL, loop0, NULL);
  pthread_create (&b, NULL, loop1, NULL);

  pause ();

  pthread_join (a, NULL);
  pthread_join (b, NULL);

  return 0;
}