Re: [PATCH net-next v9 11/13] net/rds: hold connection references in lookup, sockets and c_passive

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit's attribution trailers contradict each other.

--

net/rds: hold connection references in lookup, sockets and c_passive

This patch makes struct rds_connection pointers carry real references in
rds_conn_lookup(), __rds_conn_create(), the rs->rs_conn sendmsg cache and
parent->c_passive. It also serializes the rs_conn cache and the ToS
ioctls under rs_lock, which closes a syzbot-reported KCSAN data race
between concurrent rds_sendmsg() calls on the same socket.

Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186

> Reported-by: syzbot+879c1877016972360186@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186
> Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
> [achender: substantial reimplementation for net-next: upstream has no
>  conn reaper, per-conn workers hold no references (destroy cancels
>  them synchronously before the final put), and the sendmsg cache is
>  serialized with rs_lock instead of UEK's socket flag; rewrite commit
>  message]
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

[Severity: Low]
This isn't a bug, but do the attribution trailers match each other?

The From: author is Allison Henderson. The first Signed-off-by is
Sharath Srinivasan's, though, and there is no Co-developed-by: tag for
him.

Documentation/process/submitting-patches.rst says the first SoB entry
signals primary authorship. It also says each Co-developed-by: must be
immediately followed by that co-author's SoB.

The "[achender: ...]" bracket note is the convention a handler uses when
editing someone else's patch. Because of it, the chain reads as Sharath's
patch carried by Allison.

The message also calls this a "substantial reimplementation" that is only
"Based on the Oracle UEK commits". That suggests Sharath's bare SoB
certifies code he did not write in this form.

Would it be better to pick one of these two layouts?

  - Make Sharath the author and keep the bracket note.

  - Keep Allison as the author, add "Co-developed-by: Sharath Srinivasan"
    immediately before his Signed-off-by, and drop the bracket note.

For comparison, "net/rds: split connection destroy into quiesce and
kref-governed free" and "net/rds: wait for connections to be freed on
transport unload" in the same series are authored by Sharath. They use
this same trailer layout, and there it is consistent.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008031333.1142174-1-achender%40kernel.org