[PATCH v2 0/2] unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[PATCH v2 0/2] unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[Kirill Tkhai]

When a fd owning a counter of some critical resource, say, of a mount,
it's impossible to umount that mount and disconnect related block device.
That fd may be contained in some unix socket receive queue skb.

Despite we have an interface for detecting such the sockets queues
(/proc/[PID]/fdinfo/[fd] shows non-zero scm_fds count if so) and
it's possible to kill that process to release the counter, the problem is
that there may be several processes, and it's not a good thing to kill
each of them.

This patch adds a simple interface to grab files from receive queue,
so the caller may analyze them, and even do that recursively, if grabbed
file is unix socket itself. So, the described above problem may be solved
by this ioctl() in pair with pidfd_getfd().

Note, that the existing recvmsg(,,MSG_PEEK) is not suitable for that
purpose, since it modifies peek offset inside socket, and this results
in a problem in case of examined process uses peek offset itself.
Additional ptrace freezing of that task plus ioctl(SO_PEEK_OFF) won't help
too, since that socket may relate to several tasks, and there is no
reliable and non-racy way to detect that. Also, if the caller of such
trick will die, the examined task will remain frozen forever. The new
suggested ioctl(SIOCUNIXGRABFDS) does not have such problems.

The realization of ioctl(SIOCUNIXGRABFDS) is pretty simple. The only
interesting thing is protocol with userspace. Firstly, we let userspace
to know the number of all files in receive queue skbs. Then we receive
fds one by one starting from requested offset. We return number of
received fds if there is a successfully received fd, and this number
may be less in case of error or desired fds number lack. Userspace
may detect that situations by comparison of returned value and
out.nr_all minus in.nr_skip. Looking over different variant this one
looks the best for me (I considered returning error in case of error
and there is a received fd. Also I considered returning number of
received files as one more member in struct unix_ioc_grab_fds).

v2: Add "[PATCH 1/2] fs: Export __receive_fd()" to make receive_fd_user() be visible in [2/2] patch, when af_unix is module.
---

Kirill Tkhai (2):
      fs: Export __receive_fd()
      af_unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs


 fs/file.c               |    1 +
 include/uapi/linux/un.h |   12 ++++++++
 net/unix/af_unix.c      |   70 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)

--
Signed-off-by: Kirill Tkhai <tkhai@ya.ru>

[PATCH v2 1/2] fs: Export __receive_fd()

[Kirill Tkhai]

This is needed to make receive_fd_user() available in modules, and it will be used in next patch.

Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
---
v2: New
 fs/file.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/file.c b/fs/file.c
index 3bcc1ecc314a..e45d45f1dd45 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1181,6 +1181,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
 	__receive_sock(file);
 	return new_fd;
 }
+EXPORT_SYMBOL_GPL(__receive_fd);
 
 int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
 {

[PATCH v2 2/2] af_unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[Kirill Tkhai]

When a fd owning a counter of some critical resource, say, of a mount,
it's impossible to umount that mount and disconnect related block device.
That fd may be contained in some unix socket receive queue skb.

Despite we have an interface for detecting such the sockets queues
(/proc/[PID]/fdinfo/[fd] shows non-zero scm_fds count if so) and
it's possible to kill that process to release the counter, the problem is
that there may be several processes, and it's not a good thing to kill
each of them.

This patch adds a simple interface to grab files from receive queue,
so the caller may analyze them, and even do that recursively, if grabbed
file is unix socket itself. So, the described above problem may be solved
by this ioctl() in pair with pidfd_getfd().

Note, that the existing recvmsg(,,MSG_PEEK) is not suitable for that
purpose, since it modifies peek offset inside socket, and this results
in a problem in case of examined process uses peek offset itself.
Additional ptrace freezing of that task plus ioctl(SO_PEEK_OFF) won't help
too, since that socket may relate to several tasks, and there is no
reliable and non-racy way to detect that. Also, if the caller of such
trick will die, the examined task will remain frozen forever. The new
suggested ioctl(SIOCUNIXGRABFDS) does not have such problems.

The realization of ioctl(SIOCUNIXGRABFDS) is pretty simple. The only
interesting thing is protocol with userspace. Firstly, we let userspace
to know the number of all files in receive queue skbs. Then we receive
fds one by one starting from requested offset. We return number of
received fds if there is a successfully received fd, and this number
may be less in case of error or desired fds number lack. Userspace
may detect that situations by comparison of returned value and
out.nr_all minus in.nr_skip. Looking over different variant this one
looks the best for me (I considered returning error in case of error
and there is a received fd. Also I considered returning number of
received files as one more member in struct unix_ioc_grab_fds).

Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
---
 include/uapi/linux/un.h |   12 ++++++++
 net/unix/af_unix.c      |   70 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/include/uapi/linux/un.h b/include/uapi/linux/un.h
index 0ad59dc8b686..995b358263dd 100644
--- a/include/uapi/linux/un.h
+++ b/include/uapi/linux/un.h
@@ -11,6 +11,18 @@ struct sockaddr_un {
 	char sun_path[UNIX_PATH_MAX];	/* pathname */
 };
 
+struct unix_ioc_grab_fds {
+	struct {
+		int nr_grab;
+		int nr_skip;
+		int *fds;
+	} in;
+	struct {
+		int nr_all;
+	} out;
+};
+
 #define SIOCUNIXFILE (SIOCPROTOPRIVATE + 0) /* open a socket file with O_PATH */
+#define SIOCUNIXGRABFDS (SIOCPROTOPRIVATE + 1) /* grab files from recv queue */
 
 #endif /* _LINUX_UN_H */
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index bf338b782fc4..3c7e8049eba1 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -3079,6 +3079,73 @@ static int unix_open_file(struct sock *sk)
 	return fd;
 }
 
+static int unix_ioc_grab_fds(struct sock *sk, struct unix_ioc_grab_fds __user *uarg)
+{
+	int i, todo, skip, count, all, err, done = 0;
+	struct unix_sock *u = unix_sk(sk);
+	struct unix_ioc_grab_fds arg;
+	struct sk_buff *skb = NULL;
+	struct scm_fp_list *fp;
+
+	if (copy_from_user(&arg, uarg, sizeof(arg)))
+		return -EFAULT;
+
+	skip = arg.in.nr_skip;
+	todo = arg.in.nr_grab;
+
+	if (skip < 0 || todo <= 0)
+		return -EINVAL;
+	if (mutex_lock_interruptible(&u->iolock))
+		return -EINTR;
+
+	all = atomic_read(&u->scm_stat.nr_fds);
+	err = -EFAULT;
+	/* Set uarg->out.nr_all before the first file is received. */
+	if (put_user(all, &uarg->out.nr_all))
+		goto unlock;
+	err = 0;
+	if (all <= skip)
+		goto unlock;
+	if (all - skip < todo)
+		todo = all - skip;
+	while (todo) {
+		spin_lock(&sk->sk_receive_queue.lock);
+		if (!skb)
+			skb = skb_peek(&sk->sk_receive_queue);
+		else
+			skb = skb_peek_next(skb, &sk->sk_receive_queue);
+		spin_unlock(&sk->sk_receive_queue.lock);
+
+		if (!skb)
+			goto unlock;
+
+		fp = UNIXCB(skb).fp;
+		count = fp->count;
+		if (skip >= count) {
+			skip -= count;
+			continue;
+		}
+
+		for (i = skip; i < count && todo; i++) {
+			err = receive_fd_user(fp->fp[i], &arg.in.fds[done], 0);
+			if (err < 0)
+				goto unlock;
+			done++;
+			todo--;
+		}
+		skip = 0;
+	}
+unlock:
+	mutex_unlock(&u->iolock);
+
+	/* Return number of fds (non-error) if there is a received file. */
+	if (done)
+		return done;
+	if (err < 0)
+		return err;
+	return 0;
+}
+
 static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 {
 	struct sock *sk = sock->sk;
@@ -3113,6 +3180,9 @@ static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 		}
 		break;
 #endif
+	case SIOCUNIXGRABFDS:
+		err = unix_ioc_grab_fds(sk, (struct unix_ioc_grab_fds __user *)arg);
+		break;
 	default:
 		err = -ENOIOCTLCMD;
 		break;

[PATCH v2 1/2] fs: Export __receive_fd()

[Kirill Tkhai]

This is needed to make receive_fd_user() available in modules, and it will be used in next patch.

Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
---
v2: New
 fs/file.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/file.c b/fs/file.c
index 3bcc1ecc314a..e45d45f1dd45 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1181,6 +1181,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
 	__receive_sock(file);
 	return new_fd;
 }
+EXPORT_SYMBOL_GPL(__receive_fd);
 
 int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags)
 {

[PATCH v2 2/2] af_unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[Kirill Tkhai]

When a fd owning a counter of some critical resource, say, of a mount,
it's impossible to umount that mount and disconnect related block device.
That fd may be contained in some unix socket receive queue skb.

Despite we have an interface for detecting such the sockets queues
(/proc/[PID]/fdinfo/[fd] shows non-zero scm_fds count if so) and
it's possible to kill that process to release the counter, the problem is
that there may be several processes, and it's not a good thing to kill
each of them.

This patch adds a simple interface to grab files from receive queue,
so the caller may analyze them, and even do that recursively, if grabbed
file is unix socket itself. So, the described above problem may be solved
by this ioctl() in pair with pidfd_getfd().

Note, that the existing recvmsg(,,MSG_PEEK) is not suitable for that
purpose, since it modifies peek offset inside socket, and this results
in a problem in case of examined process uses peek offset itself.
Additional ptrace freezing of that task plus ioctl(SO_PEEK_OFF) won't help
too, since that socket may relate to several tasks, and there is no
reliable and non-racy way to detect that. Also, if the caller of such
trick will die, the examined task will remain frozen forever. The new
suggested ioctl(SIOCUNIXGRABFDS) does not have such problems.

The realization of ioctl(SIOCUNIXGRABFDS) is pretty simple. The only
interesting thing is protocol with userspace. Firstly, we let userspace
to know the number of all files in receive queue skbs. Then we receive
fds one by one starting from requested offset. We return number of
received fds if there is a successfully received fd, and this number
may be less in case of error or desired fds number lack. Userspace
may detect that situations by comparison of returned value and
out.nr_all minus in.nr_skip. Looking over different variant this one
looks the best for me (I considered returning error in case of error
and there is a received fd. Also I considered returning number of
received files as one more member in struct unix_ioc_grab_fds).

Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
---
 include/uapi/linux/un.h |   12 ++++++++
 net/unix/af_unix.c      |   70 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/include/uapi/linux/un.h b/include/uapi/linux/un.h
index 0ad59dc8b686..995b358263dd 100644
--- a/include/uapi/linux/un.h
+++ b/include/uapi/linux/un.h
@@ -11,6 +11,18 @@ struct sockaddr_un {
 	char sun_path[UNIX_PATH_MAX];	/* pathname */
 };
 
+struct unix_ioc_grab_fds {
+	struct {
+		int nr_grab;
+		int nr_skip;
+		int *fds;
+	} in;
+	struct {
+		int nr_all;
+	} out;
+};
+
 #define SIOCUNIXFILE (SIOCPROTOPRIVATE + 0) /* open a socket file with O_PATH */
+#define SIOCUNIXGRABFDS (SIOCPROTOPRIVATE + 1) /* grab files from recv queue */
 
 #endif /* _LINUX_UN_H */
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index bf338b782fc4..3c7e8049eba1 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -3079,6 +3079,73 @@ static int unix_open_file(struct sock *sk)
 	return fd;
 }
 
+static int unix_ioc_grab_fds(struct sock *sk, struct unix_ioc_grab_fds __user *uarg)
+{
+	int i, todo, skip, count, all, err, done = 0;
+	struct unix_sock *u = unix_sk(sk);
+	struct unix_ioc_grab_fds arg;
+	struct sk_buff *skb = NULL;
+	struct scm_fp_list *fp;
+
+	if (copy_from_user(&arg, uarg, sizeof(arg)))
+		return -EFAULT;
+
+	skip = arg.in.nr_skip;
+	todo = arg.in.nr_grab;
+
+	if (skip < 0 || todo <= 0)
+		return -EINVAL;
+	if (mutex_lock_interruptible(&u->iolock))
+		return -EINTR;
+
+	all = atomic_read(&u->scm_stat.nr_fds);
+	err = -EFAULT;
+	/* Set uarg->out.nr_all before the first file is received. */
+	if (put_user(all, &uarg->out.nr_all))
+		goto unlock;
+	err = 0;
+	if (all <= skip)
+		goto unlock;
+	if (all - skip < todo)
+		todo = all - skip;
+	while (todo) {
+		spin_lock(&sk->sk_receive_queue.lock);
+		if (!skb)
+			skb = skb_peek(&sk->sk_receive_queue);
+		else
+			skb = skb_peek_next(skb, &sk->sk_receive_queue);
+		spin_unlock(&sk->sk_receive_queue.lock);
+
+		if (!skb)
+			goto unlock;
+
+		fp = UNIXCB(skb).fp;
+		count = fp->count;
+		if (skip >= count) {
+			skip -= count;
+			continue;
+		}
+
+		for (i = skip; i < count && todo; i++) {
+			err = receive_fd_user(fp->fp[i], &arg.in.fds[done], 0);
+			if (err < 0)
+				goto unlock;
+			done++;
+			todo--;
+		}
+		skip = 0;
+	}
+unlock:
+	mutex_unlock(&u->iolock);
+
+	/* Return number of fds (non-error) if there is a received file. */
+	if (done)
+		return done;
+	if (err < 0)
+		return err;
+	return 0;
+}
+
 static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 {
 	struct sock *sk = sock->sk;
@@ -3113,6 +3180,9 @@ static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 		}
 		break;
 #endif
+	case SIOCUNIXGRABFDS:
+		err = unix_ioc_grab_fds(sk, (struct unix_ioc_grab_fds __user *)arg);
+		break;
 	default:
 		err = -ENOIOCTLCMD;
 		break;

RE: [PATCH v2 1/2] fs: Export __receive_fd()

[David Laight]

From: Kirill Tkhai
> Sent: 15 August 2022 22:15
> 
> This is needed to make receive_fd_user() available in modules, and it will be used in next patch.
> 
> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
> ---
> v2: New
>  fs/file.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 3bcc1ecc314a..e45d45f1dd45 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1181,6 +1181,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
>  	__receive_sock(file);
>  	return new_fd;
>  }
> +EXPORT_SYMBOL_GPL(__receive_fd);

It doesn't seem right (to me) to be exporting a function
with a __ prefix.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2 1/2] fs: Export __receive_fd()

[Kuniyuki Iwashima]

From:   David Laight <David.Laight@ACULAB.COM>
Date:   Tue, 16 Aug 2022 08:03:14 +0000
> From: Kirill Tkhai
> > Sent: 15 August 2022 22:15
> > 
> > This is needed to make receive_fd_user() available in modules, and it will be used in next patch.
> > 
> > Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
> > ---
> > v2: New
> >  fs/file.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index 3bcc1ecc314a..e45d45f1dd45 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -1181,6 +1181,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
> >  	__receive_sock(file);
> >  	return new_fd;
> >  }
> > +EXPORT_SYMBOL_GPL(__receive_fd);
> 
> It doesn't seem right (to me) to be exporting a function
> with a __ prefix.

+1.
Now receive_fd() has inline and it's the problem.
Can we avoid this by moving receive_fd() in fs/file.c without inline and
exporting it?

RE: [PATCH v2 1/2] fs: Export __receive_fd()

[David Laight]

From: Kuniyuki Iwashima
> Sent: 16 August 2022 18:15
> 
> From:   David Laight <David.Laight@ACULAB.COM>
> Date:   Tue, 16 Aug 2022 08:03:14 +0000
> > From: Kirill Tkhai
> > > Sent: 15 August 2022 22:15
> > >
> > > This is needed to make receive_fd_user() available in modules, and it will be used in next patch.
> > >
> > > Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
> > > ---
> > > v2: New
> > >  fs/file.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/fs/file.c b/fs/file.c
> > > index 3bcc1ecc314a..e45d45f1dd45 100644
> > > --- a/fs/file.c
> > > +++ b/fs/file.c
> > > @@ -1181,6 +1181,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
> > >  	__receive_sock(file);
> > >  	return new_fd;
> > >  }
> > > +EXPORT_SYMBOL_GPL(__receive_fd);
> >
> > It doesn't seem right (to me) to be exporting a function
> > with a __ prefix.
> 
> +1.
> Now receive_fd() has inline and it's the problem.
> Can we avoid this by moving receive_fd() in fs/file.c without inline and
> exporting it?

It looks like it is receive_fd_user() that should be made a real
function and then exported.
__receive_fd() can then be static.
The extra function call will be noise - and the compiler may
well either tail-call it or inline different copies of __receive_fd()
into the two callers.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH v2 2/2] af_unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[Kuniyuki Iwashima]

From:   Kirill Tkhai <tkhai@ya.ru>
Date:   Tue, 16 Aug 2022 00:22:07 +0300
> When a fd owning a counter of some critical resource, say, of a mount,
> it's impossible to umount that mount and disconnect related block device.
> That fd may be contained in some unix socket receive queue skb.
> 
> Despite we have an interface for detecting such the sockets queues
> (/proc/[PID]/fdinfo/[fd] shows non-zero scm_fds count if so) and
> it's possible to kill that process to release the counter, the problem is
> that there may be several processes, and it's not a good thing to kill
> each of them.
> 
> This patch adds a simple interface to grab files from receive queue,
> so the caller may analyze them, and even do that recursively, if grabbed
> file is unix socket itself. So, the described above problem may be solved
> by this ioctl() in pair with pidfd_getfd().
> 
> Note, that the existing recvmsg(,,MSG_PEEK) is not suitable for that
> purpose, since it modifies peek offset inside socket, and this results
> in a problem in case of examined process uses peek offset itself.
> Additional ptrace freezing of that task plus ioctl(SO_PEEK_OFF) won't help
> too, since that socket may relate to several tasks, and there is no
> reliable and non-racy way to detect that. Also, if the caller of such
> trick will die, the examined task will remain frozen forever. The new
> suggested ioctl(SIOCUNIXGRABFDS) does not have such problems.
> 
> The realization of ioctl(SIOCUNIXGRABFDS) is pretty simple. The only
> interesting thing is protocol with userspace. Firstly, we let userspace
> to know the number of all files in receive queue skbs.

Before calling SIOCUNIXGRABFDS, we have to know nr_all by reading
/proc/[PID]/fdinfo/[fd] to properly set nr_grab and nr_skip?

I'd use the same interface so that we can get it by passing 0 to
todo.


> Then we receive
> fds one by one starting from requested offset. We return number of
> received fds if there is a successfully received fd, and this number
> may be less in case of error or desired fds number lack. Userspace
> may detect that situations by comparison of returned value and
> out.nr_all minus in.nr_skip. Looking over different variant this one
> looks the best for me (I considered returning error in case of error
> and there is a received fd. Also I considered returning number of
> received files as one more member in struct unix_ioc_grab_fds).
> 
> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
> ---
>  include/uapi/linux/un.h |   12 ++++++++
>  net/unix/af_unix.c      |   70 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/include/uapi/linux/un.h b/include/uapi/linux/un.h
> index 0ad59dc8b686..995b358263dd 100644
> --- a/include/uapi/linux/un.h
> +++ b/include/uapi/linux/un.h
> @@ -11,6 +11,18 @@ struct sockaddr_un {
>  	char sun_path[UNIX_PATH_MAX];	/* pathname */
>  };
>  
> +struct unix_ioc_grab_fds {
> +	struct {
> +		int nr_grab;
> +		int nr_skip;

We can save the first validation by using unsigned int.


> +		int *fds;
> +	} in;
> +	struct {
> +		int nr_all;

unsigned int?


> +	} out;
> +};
> +
>  #define SIOCUNIXFILE (SIOCPROTOPRIVATE + 0) /* open a socket file with O_PATH */
> +#define SIOCUNIXGRABFDS (SIOCPROTOPRIVATE + 1) /* grab files from recv queue */
>  
>  #endif /* _LINUX_UN_H */
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index bf338b782fc4..3c7e8049eba1 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -3079,6 +3079,73 @@ static int unix_open_file(struct sock *sk)
>  	return fd;
>  }
>  
> +static int unix_ioc_grab_fds(struct sock *sk, struct unix_ioc_grab_fds __user *uarg)
> +{
> +	int i, todo, skip, count, all, err, done = 0;
> +	struct unix_sock *u = unix_sk(sk);
> +	struct unix_ioc_grab_fds arg;
> +	struct sk_buff *skb = NULL;
> +	struct scm_fp_list *fp;
> +
> +	if (copy_from_user(&arg, uarg, sizeof(arg)))
> +		return -EFAULT;
> +
> +	skip = arg.in.nr_skip;
> +	todo = arg.in.nr_grab;
> +
> +	if (skip < 0 || todo <= 0)

If we accept 0 as a special value for todo:

	if (skip < 0 || todo < 0)

And, unsigned int saves this.


> +		return -EINVAL;
> +	if (mutex_lock_interruptible(&u->iolock))
> +		return -EINTR;
> +
> +	all = atomic_read(&u->scm_stat.nr_fds);
> +	err = -EFAULT;
> +	/* Set uarg->out.nr_all before the first file is received. */
> +	if (put_user(all, &uarg->out.nr_all))
> +		goto unlock;
> +	err = 0;
> +	if (all <= skip)

If we accept 0 as a special value for todo:

	if (all <= skip || !todo)

> +		goto unlock;
> +	if (all - skip < todo)
> +		todo = all - skip;
> +	while (todo) {
> +		spin_lock(&sk->sk_receive_queue.lock);
> +		if (!skb)
> +			skb = skb_peek(&sk->sk_receive_queue);
> +		else
> +			skb = skb_peek_next(skb, &sk->sk_receive_queue);
> +		spin_unlock(&sk->sk_receive_queue.lock);
> +
> +		if (!skb)
> +			goto unlock;
> +
> +		fp = UNIXCB(skb).fp;
> +		count = fp->count;
> +		if (skip >= count) {
> +			skip -= count;
> +			continue;
> +		}
> +
> +		for (i = skip; i < count && todo; i++) {
> +			err = receive_fd_user(fp->fp[i], &arg.in.fds[done], 0);
> +			if (err < 0)
> +				goto unlock;
> +			done++;
> +			todo--;
> +		}
> +		skip = 0;
> +	}
> +unlock:
> +	mutex_unlock(&u->iolock);
> +
> +	/* Return number of fds (non-error) if there is a received file. */
> +	if (done)
> +		return done;

	return err;

You set 0 to err before the loop.

> +	if (err < 0)
> +		return err;
> +	return 0;
> +}
> +
>  static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>  {
>  	struct sock *sk = sock->sk;
> @@ -3113,6 +3180,9 @@ static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>  		}
>  		break;
>  #endif
> +	case SIOCUNIXGRABFDS:
> +		err = unix_ioc_grab_fds(sk, (struct unix_ioc_grab_fds __user *)arg);
> +		break;
>  	default:
>  		err = -ENOIOCTLCMD;
>  		break;

Re: [PATCH v2 0/2] unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[Al Viro]

On Tue, Aug 16, 2022 at 12:13:16AM +0300, Kirill Tkhai wrote:
> When a fd owning a counter of some critical resource, say, of a mount,
> it's impossible to umount that mount and disconnect related block device.
> That fd may be contained in some unix socket receive queue skb.
> 
> Despite we have an interface for detecting such the sockets queues
> (/proc/[PID]/fdinfo/[fd] shows non-zero scm_fds count if so) and
> it's possible to kill that process to release the counter, the problem is
> that there may be several processes, and it's not a good thing to kill
> each of them.
> 
> This patch adds a simple interface to grab files from receive queue,
> so the caller may analyze them, and even do that recursively, if grabbed
> file is unix socket itself. So, the described above problem may be solved
> by this ioctl() in pair with pidfd_getfd().
> 
> Note, that the existing recvmsg(,,MSG_PEEK) is not suitable for that
> purpose, since it modifies peek offset inside socket, and this results
> in a problem in case of examined process uses peek offset itself.
> Additional ptrace freezing of that task plus ioctl(SO_PEEK_OFF) won't help
> too, since that socket may relate to several tasks, and there is no
> reliable and non-racy way to detect that. Also, if the caller of such
> trick will die, the examined task will remain frozen forever. The new
> suggested ioctl(SIOCUNIXGRABFDS) does not have such problems.
> 
> The realization of ioctl(SIOCUNIXGRABFDS) is pretty simple. The only
> interesting thing is protocol with userspace. Firstly, we let userspace
> to know the number of all files in receive queue skbs. Then we receive
> fds one by one starting from requested offset. We return number of
> received fds if there is a successfully received fd, and this number
> may be less in case of error or desired fds number lack. Userspace
> may detect that situations by comparison of returned value and
> out.nr_all minus in.nr_skip. Looking over different variant this one
> looks the best for me (I considered returning error in case of error
> and there is a received fd. Also I considered returning number of
> received files as one more member in struct unix_ioc_grab_fds).

IMO it's a bad interface.  Take a good look at the logics in scan_children();
TCP_LISTEN side is there for reason.  And your magical ioctl won't help
with that case.

RE: [PATCH v2 1/2] fs: Export __receive_fd()

[Kuniyuki Iwashima]

From:   David Laight <David.Laight@ACULAB.COM>
Date:   Tue, 16 Aug 2022 17:29:53 +0000
> From: Kuniyuki Iwashima
> > Sent: 16 August 2022 18:15
> > 
> > From:   David Laight <David.Laight@ACULAB.COM>
> > Date:   Tue, 16 Aug 2022 08:03:14 +0000
> > > From: Kirill Tkhai
> > > > Sent: 15 August 2022 22:15
> > > >
> > > > This is needed to make receive_fd_user() available in modules, and it will be used in next patch.
> > > >
> > > > Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
> > > > ---
> > > > v2: New
> > > >  fs/file.c |    1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/fs/file.c b/fs/file.c
> > > > index 3bcc1ecc314a..e45d45f1dd45 100644
> > > > --- a/fs/file.c
> > > > +++ b/fs/file.c
> > > > @@ -1181,6 +1181,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
> > > >  	__receive_sock(file);
> > > >  	return new_fd;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(__receive_fd);
> > >
> > > It doesn't seem right (to me) to be exporting a function
> > > with a __ prefix.
> > 
> > +1.
> > Now receive_fd() has inline and it's the problem.
> > Can we avoid this by moving receive_fd() in fs/file.c without inline and
> > exporting it?
> 
> It looks like it is receive_fd_user() that should be made a real
> function and then exported.

Right, I did wrong copy-and-paste :p


> __receive_fd() can then be static.
> The extra function call will be noise - and the compiler may
> well either tail-call it or inline different copies of __receive_fd()
> into the two callers.

Re: [PATCH v2 0/2] unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[Kirill Tkhai]

On 16.08.2022 20:42, Al Viro wrote:
> On Tue, Aug 16, 2022 at 12:13:16AM +0300, Kirill Tkhai wrote:
>> When a fd owning a counter of some critical resource, say, of a mount,
>> it's impossible to umount that mount and disconnect related block device.
>> That fd may be contained in some unix socket receive queue skb.
>>
>> Despite we have an interface for detecting such the sockets queues
>> (/proc/[PID]/fdinfo/[fd] shows non-zero scm_fds count if so) and
>> it's possible to kill that process to release the counter, the problem is
>> that there may be several processes, and it's not a good thing to kill
>> each of them.
>>
>> This patch adds a simple interface to grab files from receive queue,
>> so the caller may analyze them, and even do that recursively, if grabbed
>> file is unix socket itself. So, the described above problem may be solved
>> by this ioctl() in pair with pidfd_getfd().
>>
>> Note, that the existing recvmsg(,,MSG_PEEK) is not suitable for that
>> purpose, since it modifies peek offset inside socket, and this results
>> in a problem in case of examined process uses peek offset itself.
>> Additional ptrace freezing of that task plus ioctl(SO_PEEK_OFF) won't help
>> too, since that socket may relate to several tasks, and there is no
>> reliable and non-racy way to detect that. Also, if the caller of such
>> trick will die, the examined task will remain frozen forever. The new
>> suggested ioctl(SIOCUNIXGRABFDS) does not have such problems.
>>
>> The realization of ioctl(SIOCUNIXGRABFDS) is pretty simple. The only
>> interesting thing is protocol with userspace. Firstly, we let userspace
>> to know the number of all files in receive queue skbs. Then we receive
>> fds one by one starting from requested offset. We return number of
>> received fds if there is a successfully received fd, and this number
>> may be less in case of error or desired fds number lack. Userspace
>> may detect that situations by comparison of returned value and
>> out.nr_all minus in.nr_skip. Looking over different variant this one
>> looks the best for me (I considered returning error in case of error
>> and there is a received fd. Also I considered returning number of
>> received files as one more member in struct unix_ioc_grab_fds).
> 
> IMO it's a bad interface.  Take a good look at the logics in scan_children();
> TCP_LISTEN side is there for reason.  And your magical ioctl won't help
> with that case.

Good catch, thank you! I'll extend it to handle that case too. And your words
bring to mind that fdinfo's scm_fds is also wrong for TCP_LISTEN sockets.
I've sent separate patch with you CC'ed to fix that problem.

Kirill

Re: [PATCH v2 1/2] fs: Export __receive_fd()

[Kirill Tkhai]

On 16.08.2022 11:03, David Laight wrote:
> From: Kirill Tkhai
>> Sent: 15 August 2022 22:15
>>
>> This is needed to make receive_fd_user() available in modules, and it will be used in next patch.
>>
>> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
>> ---
>> v2: New
>>  fs/file.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/file.c b/fs/file.c
>> index 3bcc1ecc314a..e45d45f1dd45 100644
>> --- a/fs/file.c
>> +++ b/fs/file.c
>> @@ -1181,6 +1181,7 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
>>  	__receive_sock(file);
>>  	return new_fd;
>>  }
>> +EXPORT_SYMBOL_GPL(__receive_fd);
> 
> It doesn't seem right (to me) to be exporting a function
> with a __ prefix.

I don't think so.

$git grep "EXPORT_SYMBOL(__\|EXPORT_SYMBOL_GPL(__" | wc -l
1649

Re: [PATCH v2 2/2] af_unix: Add ioctl(SIOCUNIXGRABFDS) to grab files of receive queue skbs

[Kirill Tkhai]

On 16.08.2022 20:31, Kuniyuki Iwashima wrote:
> From:   Kirill Tkhai <tkhai@ya.ru>
> Date:   Tue, 16 Aug 2022 00:22:07 +0300
>> When a fd owning a counter of some critical resource, say, of a mount,
>> it's impossible to umount that mount and disconnect related block device.
>> That fd may be contained in some unix socket receive queue skb.
>>
>> Despite we have an interface for detecting such the sockets queues
>> (/proc/[PID]/fdinfo/[fd] shows non-zero scm_fds count if so) and
>> it's possible to kill that process to release the counter, the problem is
>> that there may be several processes, and it's not a good thing to kill
>> each of them.
>>
>> This patch adds a simple interface to grab files from receive queue,
>> so the caller may analyze them, and even do that recursively, if grabbed
>> file is unix socket itself. So, the described above problem may be solved
>> by this ioctl() in pair with pidfd_getfd().
>>
>> Note, that the existing recvmsg(,,MSG_PEEK) is not suitable for that
>> purpose, since it modifies peek offset inside socket, and this results
>> in a problem in case of examined process uses peek offset itself.
>> Additional ptrace freezing of that task plus ioctl(SO_PEEK_OFF) won't help
>> too, since that socket may relate to several tasks, and there is no
>> reliable and non-racy way to detect that. Also, if the caller of such
>> trick will die, the examined task will remain frozen forever. The new
>> suggested ioctl(SIOCUNIXGRABFDS) does not have such problems.
>>
>> The realization of ioctl(SIOCUNIXGRABFDS) is pretty simple. The only
>> interesting thing is protocol with userspace. Firstly, we let userspace
>> to know the number of all files in receive queue skbs.
> 
> Before calling SIOCUNIXGRABFDS, we have to know nr_all by reading
> /proc/[PID]/fdinfo/[fd] to properly set nr_grab and nr_skip?
> 
> I'd use the same interface so that we can get it by passing 0 to
> todo.

Yes, this make sense 
 
>> Then we receive
>> fds one by one starting from requested offset. We return number of
>> received fds if there is a successfully received fd, and this number
>> may be less in case of error or desired fds number lack. Userspace
>> may detect that situations by comparison of returned value and
>> out.nr_all minus in.nr_skip. Looking over different variant this one
>> looks the best for me (I considered returning error in case of error
>> and there is a received fd. Also I considered returning number of
>> received files as one more member in struct unix_ioc_grab_fds).
>>
>> Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
>> ---
>>  include/uapi/linux/un.h |   12 ++++++++
>>  net/unix/af_unix.c      |   70 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 82 insertions(+)
>>
>> diff --git a/include/uapi/linux/un.h b/include/uapi/linux/un.h
>> index 0ad59dc8b686..995b358263dd 100644
>> --- a/include/uapi/linux/un.h
>> +++ b/include/uapi/linux/un.h
>> @@ -11,6 +11,18 @@ struct sockaddr_un {
>>  	char sun_path[UNIX_PATH_MAX];	/* pathname */
>>  };
>>  
>> +struct unix_ioc_grab_fds {
>> +	struct {
>> +		int nr_grab;
>> +		int nr_skip;
> 
> We can save the first validation by using unsigned int.

This int was for uniformity for fd type. But since this is not a strict connection,
we may use unsigned
 
>> +		int *fds;
>> +	} in;
>> +	struct {
>> +		int nr_all;
> 
> unsigned int?
> 
> 
>> +	} out;
>> +};
>> +
>>  #define SIOCUNIXFILE (SIOCPROTOPRIVATE + 0) /* open a socket file with O_PATH */
>> +#define SIOCUNIXGRABFDS (SIOCPROTOPRIVATE + 1) /* grab files from recv queue */
>>  
>>  #endif /* _LINUX_UN_H */
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index bf338b782fc4..3c7e8049eba1 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -3079,6 +3079,73 @@ static int unix_open_file(struct sock *sk)
>>  	return fd;
>>  }
>>  
>> +static int unix_ioc_grab_fds(struct sock *sk, struct unix_ioc_grab_fds __user *uarg)
>> +{
>> +	int i, todo, skip, count, all, err, done = 0;
>> +	struct unix_sock *u = unix_sk(sk);
>> +	struct unix_ioc_grab_fds arg;
>> +	struct sk_buff *skb = NULL;
>> +	struct scm_fp_list *fp;
>> +
>> +	if (copy_from_user(&arg, uarg, sizeof(arg)))
>> +		return -EFAULT;
>> +
>> +	skip = arg.in.nr_skip;
>> +	todo = arg.in.nr_grab;
>> +
>> +	if (skip < 0 || todo <= 0)
> 
> If we accept 0 as a special value for todo:
> 
> 	if (skip < 0 || todo < 0)
> 
> And, unsigned int saves this.
> 
> 
>> +		return -EINVAL;
>> +	if (mutex_lock_interruptible(&u->iolock))
>> +		return -EINTR;
>> +
>> +	all = atomic_read(&u->scm_stat.nr_fds);
>> +	err = -EFAULT;
>> +	/* Set uarg->out.nr_all before the first file is received. */
>> +	if (put_user(all, &uarg->out.nr_all))
>> +		goto unlock;
>> +	err = 0;
>> +	if (all <= skip)
> 
> If we accept 0 as a special value for todo:
> 
> 	if (all <= skip || !todo)
> 
>> +		goto unlock;
>> +	if (all - skip < todo)
>> +		todo = all - skip;
>> +	while (todo) {
>> +		spin_lock(&sk->sk_receive_queue.lock);
>> +		if (!skb)
>> +			skb = skb_peek(&sk->sk_receive_queue);
>> +		else
>> +			skb = skb_peek_next(skb, &sk->sk_receive_queue);
>> +		spin_unlock(&sk->sk_receive_queue.lock);
>> +
>> +		if (!skb)
>> +			goto unlock;
>> +
>> +		fp = UNIXCB(skb).fp;
>> +		count = fp->count;
>> +		if (skip >= count) {
>> +			skip -= count;
>> +			continue;
>> +		}
>> +
>> +		for (i = skip; i < count && todo; i++) {
>> +			err = receive_fd_user(fp->fp[i], &arg.in.fds[done], 0);
>> +			if (err < 0)
>> +				goto unlock;
>> +			done++;
>> +			todo--;
>> +		}
>> +		skip = 0;
>> +	}
>> +unlock:
>> +	mutex_unlock(&u->iolock);
>> +
>> +	/* Return number of fds (non-error) if there is a received file. */
>> +	if (done)
>> +		return done;
> 
> 	return err;
> 
> You set 0 to err before the loop.
> 
>> +	if (err < 0)
>> +		return err;
>> +	return 0;
>> +}
>> +
>>  static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>>  {
>>  	struct sock *sk = sock->sk;
>> @@ -3113,6 +3180,9 @@ static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>>  		}
>>  		break;
>>  #endif
>> +	case SIOCUNIXGRABFDS:
>> +		err = unix_ioc_grab_fds(sk, (struct unix_ioc_grab_fds __user *)arg);
>> +		break;
>>  	default:
>>  		err = -ENOIOCTLCMD;
>>  		break;