| Message ID | 20230807081225.816199-1-david@readahead.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b6f79e826fbd26e91e2fb28070484634cacdeb26 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/unix: use consistent error code in SO_PEERPIDFD | expand |
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 >
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>
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>
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.
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!
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);
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(-)