[PATCH] net/unix: use consistent error code in SO_PEERPIDFD

[PATCH] net/unix: use consistent error code in SO_PEERPIDFD

[David Rheinsberg]

Change the new (unreleased) SO_PEERPIDFD sockopt to return ENODATA
rather than ESRCH if a socket type does not support remote peer-PID
queries.

Currently, SO_PEERPIDFD returns ESRCH when the socket in question is
not an AF_UNIX socket. This is quite unexpected, given that one would
assume ESRCH means the peer process already exited and thus cannot be
found. However, in that case the sockopt actually returns EINVAL (via
pidfd_prepare()). This is rather inconsistent with other syscalls, which
usually return ESRCH if a given PID refers to a non-existant process.

This changes SO_PEERPIDFD to return ENODATA instead. This is also what
SO_PEERGROUPS returns, and thus keeps a consistent behavior across
sockopts.

Note that this code is returned in 2 cases: First, if the socket type is
not AF_UNIX, and secondly if the socket was not yet connected. In both
cases ENODATA seems suitable.

Signed-off-by: David Rheinsberg <david@readahead.eu>
---
Hi!

The SO_PEERPIDFD sockopt has been queued for 6.5, so hopefully we can
get that in before the release?

Thanks
David

 net/core/sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 6d4f28efe29a..732fc37a4771 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1778,7 +1778,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
 		spin_unlock(&sk->sk_peer_lock);
 
 		if (!peer_pid)
-			return -ESRCH;
+			return -ENODATA;
 
 		pidfd = pidfd_prepare(peer_pid, 0, &pidfd_file);
 		put_pid(peer_pid);
-- 
2.41.0

Re: [PATCH] net/unix: use consistent error code in SO_PEERPIDFD

[Alexander Mikhalitsyn]

On Mon, Aug 7, 2023 at 10:12 AM David Rheinsberg <david@readahead.eu> wrote:
>
> Change the new (unreleased) SO_PEERPIDFD sockopt to return ENODATA
> rather than ESRCH if a socket type does not support remote peer-PID
> queries.
>
> Currently, SO_PEERPIDFD returns ESRCH when the socket in question is
> not an AF_UNIX socket. This is quite unexpected, given that one would
> assume ESRCH means the peer process already exited and thus cannot be
> found. However, in that case the sockopt actually returns EINVAL (via
> pidfd_prepare()). This is rather inconsistent with other syscalls, which
> usually return ESRCH if a given PID refers to a non-existant process.
>
> This changes SO_PEERPIDFD to return ENODATA instead. This is also what
> SO_PEERGROUPS returns, and thus keeps a consistent behavior across
> sockopts.
>
> Note that this code is returned in 2 cases: First, if the socket type is
> not AF_UNIX, and secondly if the socket was not yet connected. In both
> cases ENODATA seems suitable.
>
> Signed-off-by: David Rheinsberg <david@readahead.eu>

Hi David!

I generally agree with this. But just to be more precise, l2cap
sockets (AF_BLUETOOTH) also properly
set ->sk_peer_pid and SO_PEERPIDFD will work just fine. This thing is
not limited
only to AF_UNIX sockets.

Kind regards,
Alex

> ---
> Hi!
>
> The SO_PEERPIDFD sockopt has been queued for 6.5, so hopefully we can
> get that in before the release?
>
> Thanks
> David
>
>  net/core/sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 6d4f28efe29a..732fc37a4771 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1778,7 +1778,7 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
>                 spin_unlock(&sk->sk_peer_lock);
>
>                 if (!peer_pid)
> -                       return -ESRCH;
> +                       return -ENODATA;
>
>                 pidfd = pidfd_prepare(peer_pid, 0, &pidfd_file);
>                 put_pid(peer_pid);
> --
> 2.41.0
>

Re: [PATCH] net/unix: use consistent error code in SO_PEERPIDFD

[Christian Brauner]

On Mon, Aug 07, 2023 at 10:12:25AM +0200, David Rheinsberg wrote:
> Change the new (unreleased) SO_PEERPIDFD sockopt to return ENODATA
> rather than ESRCH if a socket type does not support remote peer-PID
> queries.
> 
> Currently, SO_PEERPIDFD returns ESRCH when the socket in question is
> not an AF_UNIX socket. This is quite unexpected, given that one would
> assume ESRCH means the peer process already exited and thus cannot be
> found. However, in that case the sockopt actually returns EINVAL (via
> pidfd_prepare()). This is rather inconsistent with other syscalls, which
> usually return ESRCH if a given PID refers to a non-existant process.
> 
> This changes SO_PEERPIDFD to return ENODATA instead. This is also what
> SO_PEERGROUPS returns, and thus keeps a consistent behavior across
> sockopts.
> 
> Note that this code is returned in 2 cases: First, if the socket type is
> not AF_UNIX, and secondly if the socket was not yet connected. In both
> cases ENODATA seems suitable.
> 
> Signed-off-by: David Rheinsberg <david@readahead.eu>
> ---
> Hi!
> 
> The SO_PEERPIDFD sockopt has been queued for 6.5, so hopefully we can
> get that in before the release?

Shouldn't be an issue afaict.

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH] net/unix: use consistent error code in SO_PEERPIDFD

[Luca Boccassi]

On Mon, 7 Aug 2023 at 09:13, David Rheinsberg <david@readahead.eu> wrote:
>
> Change the new (unreleased) SO_PEERPIDFD sockopt to return ENODATA
> rather than ESRCH if a socket type does not support remote peer-PID
> queries.
>
> Currently, SO_PEERPIDFD returns ESRCH when the socket in question is
> not an AF_UNIX socket. This is quite unexpected, given that one would
> assume ESRCH means the peer process already exited and thus cannot be
> found. However, in that case the sockopt actually returns EINVAL (via
> pidfd_prepare()). This is rather inconsistent with other syscalls, which
> usually return ESRCH if a given PID refers to a non-existant process.
>
> This changes SO_PEERPIDFD to return ENODATA instead. This is also what
> SO_PEERGROUPS returns, and thus keeps a consistent behavior across
> sockopts.
>
> Note that this code is returned in 2 cases: First, if the socket type is
> not AF_UNIX, and secondly if the socket was not yet connected. In both
> cases ENODATA seems suitable.
>
> Signed-off-by: David Rheinsberg <david@readahead.eu>
> ---
> Hi!
>
> The SO_PEERPIDFD sockopt has been queued for 6.5, so hopefully we can
> get that in before the release?
>
> Thanks
> David
>
>  net/core/sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

As one of the first users of this new API, I am fine with this for 6.5, thanks.

Acked-by: Luca Boccassi <bluca@debian.org>

Re: [PATCH] net/unix: use consistent error code in SO_PEERPIDFD

[Simon Horman]

On Mon, Aug 07, 2023 at 10:12:25AM +0200, David Rheinsberg wrote:
> Change the new (unreleased) SO_PEERPIDFD sockopt to return ENODATA
> rather than ESRCH if a socket type does not support remote peer-PID
> queries.
> 
> Currently, SO_PEERPIDFD returns ESRCH when the socket in question is
> not an AF_UNIX socket. This is quite unexpected, given that one would
> assume ESRCH means the peer process already exited and thus cannot be
> found. However, in that case the sockopt actually returns EINVAL (via
> pidfd_prepare()). This is rather inconsistent with other syscalls, which
> usually return ESRCH if a given PID refers to a non-existant process.
> 
> This changes SO_PEERPIDFD to return ENODATA instead. This is also what
> SO_PEERGROUPS returns, and thus keeps a consistent behavior across
> sockopts.
> 
> Note that this code is returned in 2 cases: First, if the socket type is
> not AF_UNIX, and secondly if the socket was not yet connected. In both
> cases ENODATA seems suitable.
> 
> Signed-off-by: David Rheinsberg <david@readahead.eu>
> ---
> Hi!
> 
> The SO_PEERPIDFD sockopt has been queued for 6.5, so hopefully we can
> get that in before the release?
> 
> Thanks
> David

As a fix, it should probably have a fixes tag.
This one seems appropriate.

Fixes: 7b26952a91cf ("net: core: add getsockopt SO_PEERPIDFD")

And the patch should be targeted at net

	Subject: [PATCH net] ...

It's probably not necessary to repost just to address these minor points.
But please consider them if you need to post a v2 for some other reason.

Re: [PATCH] net/unix: use consistent error code in SO_PEERPIDFD

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  7 Aug 2023 10:12:25 +0200 you wrote:
> Change the new (unreleased) SO_PEERPIDFD sockopt to return ENODATA
> rather than ESRCH if a socket type does not support remote peer-PID
> queries.
> 
> Currently, SO_PEERPIDFD returns ESRCH when the socket in question is
> not an AF_UNIX socket. This is quite unexpected, given that one would
> assume ESRCH means the peer process already exited and thus cannot be
> found. However, in that case the sockopt actually returns EINVAL (via
> pidfd_prepare()). This is rather inconsistent with other syscalls, which
> usually return ESRCH if a given PID refers to a non-existant process.
> 
> [...]

Here is the summary with links:
  - net/unix: use consistent error code in SO_PEERPIDFD
    https://git.kernel.org/netdev/net/c/b6f79e826fbd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html