[PATCH net] netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

[PATCH net] netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

[Maoyi Xie]

__scm_recv_common() translates uid and gid into the reader's user
namespace but copies the pid as is. AF_UNIX gets away with that because
unix_skb_to_scm() re-renders the pid with pid_vnr() at recvmsg time.
netlink_sendmsg() renders it in the sender's namespace and stores a bare
u32, so a reader in another pid namespace sees a number from a namespace
it is not in. The sender chooses that number. An unprivileged sender in a
child namespace made the receiver see pid 300.

Carry the sender's struct pid in NETLINK_CB and hand it to scm_set_cred()
in netlink_recvmsg(), the way af_unix does. netlink_skb_set_owner_r()
takes the reference and netlink_skb_destructor() drops it. A reader in a
namespace the sender has no pid in now gets 0, like AF_UNIX.

I found this with a CodeQL checker. I used Claude to help write the
reproducers. The ones that reproduce the bug run unprivileged and need no
kernel changes. Tested on net with KASAN and lockdep, no reports. The tree
has no netlink SCM selftest.

Fixes: b488893a390e ("pid namespaces: changes to show virtual ids to user")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5 codeql
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
 include/linux/netlink.h  |  9 +++++++++
 net/netlink/af_netlink.c | 30 ++++++++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 882e9c1b6c1dcc..26266754e27c14 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -30,6 +30,15 @@ struct netlink_skb_parms {
 	struct sock		*sk;
 	bool			nsid_is_set;
 	int			nsid;
+	/*
+	 * Sender's struct pid.  netlink_sendmsg() stores a borrowed pointer
+	 * taken from its own scm_cookie.  netlink_skb_set_owner_r() takes a
+	 * reference when it takes ownership of the skb for a receiver, and
+	 * netlink_skb_destructor() drops that reference.  A clone starts out
+	 * borrowing again, because __skb_clone() clears both skb->sk and
+	 * skb->destructor.  NULL for a kernel generated skb.
+	 */
+	struct pid		*pid;
 };
 
 #define NETLINK_CB(skb)		(*(struct netlink_skb_parms*)&((skb)->cb))
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index e6b1d9758c9c92..170d90d472a0db 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -379,13 +379,24 @@ static void netlink_skb_destructor(struct sk_buff *skb)
 
 		skb->head = NULL;
 	}
-	if (skb->sk != NULL)
+	if (skb->sk) {
+		/*
+		 * The reference is held for as long as skb->sk is set, taken
+		 * in netlink_skb_set_owner_r() and dropped here.  The pointer
+		 * is left in place: do_one_broadcast() orphans an skb one
+		 * listener owned and hands it to the next, which takes its
+		 * own reference, and the sender's scm_cookie keeps the pid
+		 * alive across the whole broadcast.
+		 */
+		put_pid(NETLINK_CB(skb).pid);
 		sock_rfree(skb);
+	}
 }
 
 static void netlink_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 {
 	WARN_ON(skb->sk != NULL);
+	NETLINK_CB(skb).pid = get_pid(NETLINK_CB(skb).pid);
 	skb->sk = sk;
 	skb->destructor = netlink_skb_destructor;
 	sk_mem_charge(sk, skb->truesize);
@@ -1880,6 +1891,13 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	NETLINK_CB(skb).dst_group = dst_group;
 	NETLINK_CB(skb).creds	= scm.creds;
 	NETLINK_CB(skb).flags	= netlink_skb_flags;
+	/*
+	 * Borrowed here.  scm_destroy() below drops the scm_cookie's own
+	 * reference, and every delivery in between is synchronous, so the
+	 * pointer stays valid until netlink_skb_set_owner_r() takes a
+	 * reference of its own.
+	 */
+	NETLINK_CB(skb).pid	= scm.pid;
 
 	err = -EFAULT;
 	if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
@@ -1971,7 +1989,15 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 		netlink_cmsg_listen_all_nsid(sk, msg, skb);
 
 	memset(&scm, 0, sizeof(scm));
-	scm.creds = *NETLINK_CREDS(skb);
+	/*
+	 * Render the sender's pid in the reader's pid namespace, the way
+	 * unix_skb_to_scm() does through scm_set_cred().  A NULL pid gives 0,
+	 * so a control block that lost its reference reports "unknown" rather
+	 * than the sender's own untranslated number.  scm_recv() below drops
+	 * the reference taken here on both of its paths.
+	 */
+	scm_set_cred(&scm, NETLINK_CB(skb).pid, NETLINK_CREDS(skb)->uid,
+		     NETLINK_CREDS(skb)->gid);
 	if (flags & MSG_TRUNC)
 		copied = data_skb->len;
 
-- 
2.34.1

Re: [PATCH net] netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

[Eric Dumazet]

On Fri, Sep 4, 2026 at 4:46 PM Maoyi Xie <maoyixie.tju@gmail.com> wrote:
>
> __scm_recv_common() translates uid and gid into the reader's user
> namespace but copies the pid as is. AF_UNIX gets away with that because
> unix_skb_to_scm() re-renders the pid with pid_vnr() at recvmsg time.
> netlink_sendmsg() renders it in the sender's namespace and stores a bare
> u32, so a reader in another pid namespace sees a number from a namespace
> it is not in. The sender chooses that number. An unprivileged sender in a
> child namespace made the receiver see pid 300.
>
> Carry the sender's struct pid in NETLINK_CB and hand it to scm_set_cred()
> in netlink_recvmsg(), the way af_unix does. netlink_skb_set_owner_r()
> takes the reference and netlink_skb_destructor() drops it. A reader in a
> namespace the sender has no pid in now gets 0, like AF_UNIX.
>
> I found this with a CodeQL checker. I used Claude to help write the
> reproducers. The ones that reproduce the bug run unprivileged and need no
> kernel changes. Tested on net with KASAN and lockdep, no reports. The tree
> has no netlink SCM selftest.
>
> Fixes: b488893a390e ("pid namespaces: changes to show virtual ids to user")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5 codeql
> Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
> ---
>  include/linux/netlink.h  |  9 +++++++++
>  net/netlink/af_netlink.c | 30 ++++++++++++++++++++++++++++--
>  2 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 882e9c1b6c1dcc..26266754e27c14 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -30,6 +30,15 @@ struct netlink_skb_parms {
>         struct sock             *sk;
>         bool                    nsid_is_set;
>         int                     nsid;
> +       /*
> +        * Sender's struct pid.  netlink_sendmsg() stores a borrowed pointer
> +        * taken from its own scm_cookie.  netlink_skb_set_owner_r() takes a
> +        * reference when it takes ownership of the skb for a receiver, and
> +        * netlink_skb_destructor() drops that reference.  A clone starts out
> +        * borrowing again, because __skb_clone() clears both skb->sk and
> +        * skb->destructor.  NULL for a kernel generated skb.
> +        */
> +       struct pid              *pid;
>  };
>
>  #define NETLINK_CB(skb)                (*(struct netlink_skb_parms*)&((skb)->cb))
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index e6b1d9758c9c92..170d90d472a0db 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -379,13 +379,24 @@ static void netlink_skb_destructor(struct sk_buff *skb)
>
>                 skb->head = NULL;
>         }
> -       if (skb->sk != NULL)
> +       if (skb->sk) {
> +               /*
> +                * The reference is held for as long as skb->sk is set, taken
> +                * in netlink_skb_set_owner_r() and dropped here.  The pointer
> +                * is left in place: do_one_broadcast() orphans an skb one
> +                * listener owned and hands it to the next, which takes its
> +                * own reference, and the sender's scm_cookie keeps the pid
> +                * alive across the whole broadcast.
> +                */
> +               put_pid(NETLINK_CB(skb).pid);

I really have a bad feeling about this dance and these assumptions,
maybe this is just me.

Please include a dedicated selftest in tools/testing/selftests/net/
exercising SO_PASSCRED / SCM_CREDENTIALS
over netlink across PID namespaces to demonstrate the issue and
validate the proposed changes.

Thanks.

>                 sock_rfree(skb);
> +       }
>  }
>
>  static void netlink_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
>  {
>         WARN_ON(skb->sk != NULL);
> +       NETLINK_CB(skb).pid = get_pid(NETLINK_CB(skb).pid);
>         skb->sk = sk;
>         skb->destructor = netlink_skb_destructor;
>         sk_mem_charge(sk, skb->truesize);
> @@ -1880,6 +1891,13 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>         NETLINK_CB(skb).dst_group = dst_group;
>         NETLINK_CB(skb).creds   = scm.creds;
>         NETLINK_CB(skb).flags   = netlink_skb_flags;
> +       /*
> +        * Borrowed here.  scm_destroy() below drops the scm_cookie's own
> +        * reference, and every delivery in between is synchronous, so the
> +        * pointer stays valid until netlink_skb_set_owner_r() takes a
> +        * reference of its own.
> +        */
> +       NETLINK_CB(skb).pid     = scm.pid;
>
>         err = -EFAULT;
>         if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> @@ -1971,7 +1989,15 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>                 netlink_cmsg_listen_all_nsid(sk, msg, skb);
>
>         memset(&scm, 0, sizeof(scm));
> -       scm.creds = *NETLINK_CREDS(skb);
> +       /*
> +        * Render the sender's pid in the reader's pid namespace, the way
> +        * unix_skb_to_scm() does through scm_set_cred().  A NULL pid gives 0,
> +        * so a control block that lost its reference reports "unknown" rather
> +        * than the sender's own untranslated number.  scm_recv() below drops
> +        * the reference taken here on both of its paths.
> +        */
> +       scm_set_cred(&scm, NETLINK_CB(skb).pid, NETLINK_CREDS(skb)->uid,
> +                    NETLINK_CREDS(skb)->gid);
>         if (flags & MSG_TRUNC)
>                 copied = data_skb->len;
>
> --
> 2.34.1
>

Re: [PATCH net] netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

[Alexander Mikhalitsyn]

Am Fr., 4. Sept. 2026 um 17:48 Uhr schrieb Eric Dumazet <edumazet@google.com>:
>
> On Fri, Sep 4, 2026 at 4:46 PM Maoyi Xie <maoyixie.tju@gmail.com> wrote:
> >
> > __scm_recv_common() translates uid and gid into the reader's user
> > namespace but copies the pid as is. AF_UNIX gets away with that because
> > unix_skb_to_scm() re-renders the pid with pid_vnr() at recvmsg time.
> > netlink_sendmsg() renders it in the sender's namespace and stores a bare
> > u32, so a reader in another pid namespace sees a number from a namespace
> > it is not in. The sender chooses that number. An unprivileged sender in a
> > child namespace made the receiver see pid 300.
> >
> > Carry the sender's struct pid in NETLINK_CB and hand it to scm_set_cred()
> > in netlink_recvmsg(), the way af_unix does. netlink_skb_set_owner_r()
> > takes the reference and netlink_skb_destructor() drops it. A reader in a
> > namespace the sender has no pid in now gets 0, like AF_UNIX.
> >
> > I found this with a CodeQL checker. I used Claude to help write the
> > reproducers. The ones that reproduce the bug run unprivileged and need no
> > kernel changes. Tested on net with KASAN and lockdep, no reports. The tree
> > has no netlink SCM selftest.
> >
> > Fixes: b488893a390e ("pid namespaces: changes to show virtual ids to user")
> > Cc: stable@vger.kernel.org
> > Assisted-by: Claude:claude-opus-5 codeql
> > Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
> > ---
> >  include/linux/netlink.h  |  9 +++++++++
> >  net/netlink/af_netlink.c | 30 ++++++++++++++++++++++++++++--
> >  2 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> > index 882e9c1b6c1dcc..26266754e27c14 100644
> > --- a/include/linux/netlink.h
> > +++ b/include/linux/netlink.h
> > @@ -30,6 +30,15 @@ struct netlink_skb_parms {
> >         struct sock             *sk;
> >         bool                    nsid_is_set;
> >         int                     nsid;
> > +       /*
> > +        * Sender's struct pid.  netlink_sendmsg() stores a borrowed pointer
> > +        * taken from its own scm_cookie.  netlink_skb_set_owner_r() takes a
> > +        * reference when it takes ownership of the skb for a receiver, and
> > +        * netlink_skb_destructor() drops that reference.  A clone starts out
> > +        * borrowing again, because __skb_clone() clears both skb->sk and
> > +        * skb->destructor.  NULL for a kernel generated skb.
> > +        */
> > +       struct pid              *pid;
> >  };
> >
> >  #define NETLINK_CB(skb)                (*(struct netlink_skb_parms*)&((skb)->cb))
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index e6b1d9758c9c92..170d90d472a0db 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -379,13 +379,24 @@ static void netlink_skb_destructor(struct sk_buff *skb)
> >
> >                 skb->head = NULL;
> >         }
> > -       if (skb->sk != NULL)
> > +       if (skb->sk) {
> > +               /*
> > +                * The reference is held for as long as skb->sk is set, taken
> > +                * in netlink_skb_set_owner_r() and dropped here.  The pointer
> > +                * is left in place: do_one_broadcast() orphans an skb one
> > +                * listener owned and hands it to the next, which takes its
> > +                * own reference, and the sender's scm_cookie keeps the pid
> > +                * alive across the whole broadcast.
> > +                */
> > +               put_pid(NETLINK_CB(skb).pid);
>

Dear friends,

> I really have a bad feeling about this dance and these assumptions,
> maybe this is just me.

completely agree with Eric.

1. We need to start from use-case here, because netlink sockets are
quite special in many ways.
     If you just found this using LLM and now we are going to change a
behavior that was there for more than 22 years
     ( just look
https://github.com/torvalds/linux/blame/d679c5324d9a87c6295f56c2dea52d5f68834f41/include/linux/netlink.h#L165
)
2. If you start tracking struct pid (which is a good idea
theoretically, if we ignore 1.), then we should do it a bit smarter
and
    make use pidfs API [pidfs_register_pid() function] like we do it
in unix_maybe_add_creds() to make sure that we properly support
    dead pidfds too.

Kind regards,
Alex

>
> Please include a dedicated selftest in tools/testing/selftests/net/
> exercising SO_PASSCRED / SCM_CREDENTIALS
> over netlink across PID namespaces to demonstrate the issue and
> validate the proposed changes.
>
> Thanks.
>
> >                 sock_rfree(skb);
> > +       }
> >  }
> >
> >  static void netlink_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
> >  {
> >         WARN_ON(skb->sk != NULL);
> > +       NETLINK_CB(skb).pid = get_pid(NETLINK_CB(skb).pid);
> >         skb->sk = sk;
> >         skb->destructor = netlink_skb_destructor;
> >         sk_mem_charge(sk, skb->truesize);
> > @@ -1880,6 +1891,13 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> >         NETLINK_CB(skb).dst_group = dst_group;
> >         NETLINK_CB(skb).creds   = scm.creds;
> >         NETLINK_CB(skb).flags   = netlink_skb_flags;
> > +       /*
> > +        * Borrowed here.  scm_destroy() below drops the scm_cookie's own
> > +        * reference, and every delivery in between is synchronous, so the
> > +        * pointer stays valid until netlink_skb_set_owner_r() takes a
> > +        * reference of its own.
> > +        */
> > +       NETLINK_CB(skb).pid     = scm.pid;
> >
> >         err = -EFAULT;
> >         if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> > @@ -1971,7 +1989,15 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >                 netlink_cmsg_listen_all_nsid(sk, msg, skb);
> >
> >         memset(&scm, 0, sizeof(scm));
> > -       scm.creds = *NETLINK_CREDS(skb);
> > +       /*
> > +        * Render the sender's pid in the reader's pid namespace, the way
> > +        * unix_skb_to_scm() does through scm_set_cred().  A NULL pid gives 0,
> > +        * so a control block that lost its reference reports "unknown" rather
> > +        * than the sender's own untranslated number.  scm_recv() below drops
> > +        * the reference taken here on both of its paths.
> > +        */
> > +       scm_set_cred(&scm, NETLINK_CB(skb).pid, NETLINK_CREDS(skb)->uid,
> > +                    NETLINK_CREDS(skb)->gid);
> >         if (flags & MSG_TRUNC)
> >                 copied = data_skb->len;
> >
> > --
> > 2.34.1
> >

Re: [PATCH net] netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

[Maoyi Xie]

Please drop this patch.

Sorry for the time this cost you both. I sent it without a selftest and
without a use case, and that was the wrong order.

Eric Dumazet wrote:
> Please include a dedicated selftest in tools/testing/selftests/net/
> exercising SO_PASSCRED / SCM_CREDENTIALS
> over netlink across PID namespaces to demonstrate the issue and
> validate the proposed changes.

I wrote it as tools/testing/selftests/net/netlink_scm_pidns.c and have
not posted it, since I am asking you to drop the patch. cross_pidns
fails on 66817a979426 and passes with the patch. reader_in_child_ns goes
the other way. It passes on 66817a979426 and fails with the patch
applied. With the patch, an unprivileged sender in the parent pid
namespace makes a NETLINK_USERSOCK reader in a child namespace see pid
0. No userspace sender can produce 0 today, since find_get_pid(0) is
NULL. That is why I am asking you to drop the patch.

Alexander Mikhalitsyn wrote:
> 1. We need to start from use-case here, because netlink sockets are
> quite special in many ways.

What I measured on an unpatched kernel is that a task with no
capabilities, after unshare(CLONE_NEWUSER) and unshare(CLONE_NEWPID),
chooses the number a NETLINK_USERSOCK reader in a sibling pid namespace
is told, by picking its own position in the namespace it created. I have
not found a program that reads that number on NETLINK_USERSOCK and acts
on it. avahi does on NETLINK_ROUTE, where it accepts a message only when
the pid is 0, and an unprivileged sender that shares its netns gets
-EPERM there.

Is the sender-chosen number worth changing at all?

Alexander Mikhalitsyn wrote:
> 2. If you start tracking struct pid (which is a good idea
> theoretically, if we ignore 1.), then we should do it a bit smarter
> and
>     make use pidfs API [pidfs_register_pid() function] like we do it
> in unix_maybe_add_creds() to make sure that we properly support
>     dead pidfds too.

With the patch dropped nothing in netlink stores a struct pid, so I have
not added pidfs_register_pid(). If the answer is yes, I will start from
your point 2.

I will not post again unless one of you says it is worth changing.

Thanks.

Re: [PATCH net] netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 1 · High: 0 · Medium: 1 · Low: 1

- [Critical] Storing destructor-needed data (struct pid *) in skb->cb
  violates SKB Control Block Lifetime rules.
- [Medium] The new reference-counting scheme leaves a dangling `struct
  pid *` inside a live skb and then unconditionally re-acquires a…
- [Low] The three comments this patch adds to document its new borrowed-
  pid reference scheme contain two verifiably false statements about the…

--

netlink: render SCM_CREDENTIALS pid in the receiver's pid namespace

The patch carries the sender's struct pid in NETLINK_CB so that
netlink_recvmsg() can translate it into the reader's pid namespace with
scm_set_cred(), instead of copying the sender-rendered u32. The reference
is taken in netlink_skb_set_owner_r() and dropped in
netlink_skb_destructor().

> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 882e9c1b6c1dc..26266754e27c1 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -30,6 +30,15 @@ struct netlink_skb_parms {
>  	struct sock		*sk;
>  	bool			nsid_is_set;
>  	int			nsid;
> +	/*
> +	 * Sender's struct pid.  netlink_sendmsg() stores a borrowed pointer
> +	 * taken from its own scm_cookie.  netlink_skb_set_owner_r() takes a
> +	 * reference when it takes ownership of the skb for a receiver, and
> +	 * netlink_skb_destructor() drops that reference.  A clone starts out
> +	 * borrowing again, because __skb_clone() clears both skb->sk and
> +	 * skb->destructor.  NULL for a kernel generated skb.
> +	 */
> +	struct pid		*pid;
>  };
>  
>  #define NETLINK_CB(skb)		(*(struct netlink_skb_parms*)&((skb)->cb))
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index e6b1d9758c9c9..170d90d472a0d 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -379,13 +379,24 @@ static void netlink_skb_destructor(struct sk_buff *skb)
>  
>  		skb->head = NULL;
>  	}
> -	if (skb->sk != NULL)
> +	if (skb->sk) {
> +		/*
> +		 * The reference is held for as long as skb->sk is set, taken
> +		 * in netlink_skb_set_owner_r() and dropped here.  The pointer
> +		 * is left in place: do_one_broadcast() orphans an skb one
> +		 * listener owned and hands it to the next, which takes its
> +		 * own reference, and the sender's scm_cookie keeps the pid
> +		 * alive across the whole broadcast.
> +		 */
> +		put_pid(NETLINK_CB(skb).pid);
>  		sock_rfree(skb);
> +	}
>  }
>  
>  static void netlink_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
>  {
>  	WARN_ON(skb->sk != NULL);
> +	NETLINK_CB(skb).pid = get_pid(NETLINK_CB(skb).pid);
>  	skb->sk = sk;
>  	skb->destructor = netlink_skb_destructor;
>  	sk_mem_charge(sk, skb->truesize);

[Severity: Critical]
NETLINK_CB() is just an overlay on skb->cb, the 48-byte scratch
area that every layer an skb passes through is allowed to reuse.
Until now netlink only parked plain values and one non-refcounted
struct sock *sk there, so a clobbered cb cost at most wrong
metadata. After this patch the cb holds the *only* reference to a
struct pid for as long as the skb is owned by a receiver, and
netlink_skb_destructor() consumes it. If anything rewrites those
eight bytes between netlink_skb_set_owner_r() and the destructor
running, put_pid() executes refcount_dec_and_test() and
kmem_cache_free() on whatever now sits at that offset; the mirror
case, cb overwritten before the destructor, silently leaks the pid.
Storing destructor-needed state in skb->cb is the pattern the
networking guidance calls out precisely because the read side runs
long after the write side.

What establishes the invariant that the cb is untouched for that
whole window? For an skb parked in a netlink socket's
sk_receive_queue I believe nothing else writes cb, but the patch
adds no assertion or comment saying that, and netlink skbs do get
handed to other code: __netlink_deliver_tap_skb() clones into
dev_queue_xmit(), and netlink_skb_clone() callers such as
nl_fib_input() and nfnetlink_rcv_batch() pass clones on. Those
clones copy the pointer verbatim through __copy_skb_header() while
__skb_clone() clears skb->sk and skb->destructor, so today no
put_pid() runs on them - the safety comes from the clone never
being re-owned, not from anything the new code enforces.
netlink_skb_set_owner_r() does get_pid() on whatever byte pattern
it finds in the cb, with no way to tell a live pointer from
leftovers.

Could the reference be tied to something with a defined lifetime
instead? Taking it in netlink_sendmsg() when the cb is written, and
dropping it on every skb-free path rather than only when skb->sk
happens to be set, would at least make the ownership match the
skb's lifetime rather than the sk/destructor pairing. If keeping
the pointer in cb is the intent, please state in the commit message
which paths may see an skb with a live pid in NETLINK_CB and why no
layer on those paths rewrites cb offset 40.

Unrelated but adjacent: adding a pointer makes struct
netlink_skb_parms fill the cb exactly on 64-bit. Did you check it
against the BUILD_BUG_ON in netlink_proto_init(), and is leaving
zero slack in a shared scratch area something we want to do for a
stable backport?

[Severity: Medium]
Is it intentional to leave the pointer in the control block after
put_pid(), given that netlink_skb_set_owner_r() then does get_pid() on
whatever it finds there without any validation?

The drop-then-re-acquire window exists in tree, in do_one_broadcast():

	if (p->skb2 == NULL) {
		if (skb_shared(p->skb)) {
			p->skb2 = skb_clone(p->skb, p->allocation);
		} else {
			p->skb2 = skb_get(p->skb);
			/*
			 * skb ownership may have been set when
			 * delivered to a previous socket.
			 */
			skb_orphan(p->skb2);
		}
	}

skb_orphan() runs netlink_skb_destructor(), which drops the reference the
previous listener took, and the next listener re-increments the same
pointer through netlink_broadcast_deliver()->netlink_skb_set_owner_r().

The only thing keeping that get_pid() off a freed struct pid is a
reference in a different stack frame, scm.pid in netlink_sendmsg(),
released by scm_destroy() after both netlink_broadcast() and
netlink_unicast() have returned. Nothing in the exported entry points
(netlink_unicast(), netlink_broadcast(), netlink_broadcast_filtered())
expresses or checks that invariant, and skb_clone()/skb_copy()/
netlink_skb_clone() copy the cb, and therefore the bare pid pointer,
verbatim via __copy_skb_header(). netlink_trim() has the same shape: it
clones a shared skb and consume_skb()s the source before the clone is
re-owned, guarded only by the WARN_ON(skb->sk != NULL) above.

I could not construct a path that is live today: other senders use freshly
allocated skbs whose cb is zeroed, and the netlink_skb_clone() users I
looked at, nl_fib_input() and nfnetlink_rcv_batch(), run synchronously
while the origin skb still owns its reference. Still, would setting

	NETLINK_CB(skb).pid = NULL;

after put_pid() be preferable? A re-owned skb would then report pid 0,
which is the value the changelog already describes as acceptable for
"unknown". Alternatively, taking the reference at send time the way
unix_scm_to_skb() does would match what the changelog says is being
imitated.

> @@ -1880,6 +1891,13 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
>  	NETLINK_CB(skb).dst_group = dst_group;
>  	NETLINK_CB(skb).creds	= scm.creds;
>  	NETLINK_CB(skb).flags	= netlink_skb_flags;
> +	/*
> +	 * Borrowed here.  scm_destroy() below drops the scm_cookie's own
> +	 * reference, and every delivery in between is synchronous, so the
> +	 * pointer stays valid until netlink_skb_set_owner_r() takes a
> +	 * reference of its own.
> +	 */
> +	NETLINK_CB(skb).pid	= scm.pid;
>  
>  	err = -EFAULT;
>  	if (memcpy_from_msg(skb_put(skb, len), msg, len)) {

[ ... ]

> @@ -1971,7 +1989,15 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  		netlink_cmsg_listen_all_nsid(sk, msg, skb);
>  
>  	memset(&scm, 0, sizeof(scm));
> -	scm.creds = *NETLINK_CREDS(skb);
> +	/*
> +	 * Render the sender's pid in the reader's pid namespace, the way
> +	 * unix_skb_to_scm() does through scm_set_cred().  A NULL pid gives 0,
> +	 * so a control block that lost its reference reports "unknown" rather
> +	 * than the sender's own untranslated number.  scm_recv() below drops
> +	 * the reference taken here on both of its paths.
> +	 */
> +	scm_set_cred(&scm, NETLINK_CB(skb).pid, NETLINK_CREDS(skb)->uid,
> +		     NETLINK_CREDS(skb)->gid);

[Severity: Low]
Two of the comments this patch adds seem to describe a model the code does
not implement.

First, this comment says "A NULL pid gives 0, so a control block that lost
its reference reports 'unknown'". Which code produces that NULL? The
destructor added a few lines above states the opposite and keeps the
pointer:

	/*
	 * ... The pointer
	 * is left in place: do_one_broadcast() orphans an skb one
	 * listener owned and hands it to the next, ...
	 */
	put_pid(NETLINK_CB(skb).pid);

So a control block that "lost its reference" holds a stale non-NULL
pointer, not NULL, and scm_set_cred() would call get_pid()/pid_vnr() on
it. Should the two comments be reconciled?

Second, the netlink_sendmsg() comment says "every delivery in between is
synchronous". Does that hold for the tap path?

	__netlink_deliver_tap_skb()
		nskb = skb_clone(skb, GFP_ATOMIC);
		...
		ret = dev_queue_xmit(nskb);

__skb_clone() copies the full 48-byte cb, so the borrowed pid pointer
comes along while nskb->sk and nskb->destructor are cleared and no
reference is taken. dev_queue_xmit() can then leave the clone in an
AF_PACKET receive queue via dev_queue_xmit_nit(), or in a qdisc attached
to the nlmon device with transmission deferred to net_tx_action(), both of
which can outlive sendmsg() and the scm_destroy() that drops the sender's
only reference. The escaping copy is not overwritten by those layers,
since qdisc_skb_cb and PACKET_SKB_CB stay within the first ~28 bytes of
cb while pid sits at cb offset 40 on 64-bit.

No current consumer dereferences that escaped pointer, so this is a
comment accuracy question rather than a runtime bug, but the prose
invariant is what a later change would build on. Would it be clearer to
state the invariant that actually holds, namely that an skb only reaches
netlink_recvmsg() after netlink_skb_set_owner_r() gave it its own
reference, and that while borrowed the liveness comes solely from the
scm_cookie on netlink_sendmsg()'s stack?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904144636.3443342-1-maoyixie.tju%40gmail.com