Sparse reports the following warnings:
net/rds/af_rds.c:245:22: warning: invalid assignment: |=
net/rds/af_rds.c:245:22: left side has type restricted __poll_t
net/rds/af_rds.c:245:22: right side has type int
__poll_t is typedef'ed to __bitwise while POLLERR is defined as 0x0008,
force conversion.
net/rds/recv.c:218:42: warning: cast to restricted __be16
net/rds/recv.c:222:44: warning: cast to restricted __be32
Replace be{16,32}_to_cpu with explicit __force to force conversion. The
value remains the same either way so not a bug.
net/rds/send.c:1050:24: warning: incorrect type in argument 1 (different base types)
net/rds/send.c:1050:24: expected unsigned int [usertype] a
net/rds/send.c:1050:24: got restricted __be16 [usertype] sin6_port
net/rds/send.c:1052:24: warning: incorrect type in argument 1 (different base types)
net/rds/send.c:1052:24: expected unsigned int [usertype] a
net/rds/send.c:1052:24: got restricted __be16 [usertype] sin6_port
net/rds/send.c:1457:30: warning: incorrect type in initializer (different base types)
net/rds/send.c:1457:30: expected unsigned short [usertype] npaths
net/rds/send.c:1457:30: got restricted __be16 [usertype]
net/rds/send.c:1458:34: warning: incorrect type in initializer (different base types)
net/rds/send.c:1458:34: expected unsigned int [usertype] my_gen_num
net/rds/send.c:1458:34: got restricted __be32 [usertype]
Use correct endianness. Replace cpu_to_be{16,32} for the same reason as
above.
net/rds/connection.c:71:31: warning: incorrect type in argument 1 (different base types)
net/rds/connection.c:71:31: expected restricted __be32 const [usertype] laddr
net/rds/connection.c:71:31: got unsigned int [assigned] [usertype] lhash
net/rds/connection.c:71:41: warning: incorrect type in argument 3 (different base types)
net/rds/connection.c:71:41: expected restricted __be32 const [usertype] faddr
net/rds/connection.c:71:41: got unsigned int [assigned] [usertype] fhash
Signed-off-by: Ujwal Kundur <redacted>
---
net/rds/af_rds.c | 2 +-
net/rds/connection.c | 6 +++---
net/rds/rdma.c | 1 -
net/rds/rds.h | 2 +-
net/rds/recv.c | 4 ++--
net/rds/send.c | 4 ++--
6 files changed, 9 insertions(+), 10 deletions(-)
@@ -242,7 +242,7 @@ static __poll_t rds_poll(struct file *file, struct socket *sock,if(rs->rs_snd_bytes<rds_sk_sndbuf(rs))mask|=(EPOLLOUT|EPOLLWRNORM);if(sk->sk_err||!skb_queue_empty(&sk->sk_error_queue))-mask|=POLLERR;+mask|=(__force__poll_t)POLLERR;read_unlock_irqrestore(&rs->rs_recv_lock,flags);/* clear state any time we wake a seen-congested socket */
@@ -93,7 +93,7 @@ enum {/* Max number of multipaths per RDS connection. Must be a power of 2 */#define RDS_MPATH_WORKERS 8-#define RDS_MPATH_HASH(rs, n) (jhash_1word((rs)->rs_bound_port, \+#define RDS_MPATH_HASH(rs, n) (jhash_1word((__force __u16)(rs)->rs_bound_port, \(rs)->rs_hash_initval)&((n)-1))#define IS_CANONICAL(laddr, faddr) (htonl(laddr) < htonl(faddr))
From: Andrew Lunn <andrew@lunn.ch> Date: 2025-08-10 17:32:16
On Sun, Aug 10, 2025 at 10:41:55PM +0530, Ujwal Kundur wrote:
Sparse reports the following warnings:
net/rds/af_rds.c:245:22: warning: invalid assignment: |=
net/rds/af_rds.c:245:22: left side has type restricted __poll_t
net/rds/af_rds.c:245:22: right side has type int
__poll_t is typedef'ed to __bitwise while POLLERR is defined as 0x0008,
force conversion.
Please could you split this up, one patch per type of problem.
what the... You have lhash and fhash set to __be32 values, then
feed them to function that expects __be32 argument. Just turn
these two variables into __be32 and lose the pointless casts.
@@ -93,7 +93,7 @@ enum {/* Max number of multipaths per RDS connection. Must be a power of 2 */#define RDS_MPATH_WORKERS 8-#define RDS_MPATH_HASH(rs, n) (jhash_1word((rs)->rs_bound_port, \+#define RDS_MPATH_HASH(rs, n) (jhash_1word((__force __u16)(rs)->rs_bound_port, \(rs)->rs_hash_initval)&((n)-1))
No. It will break on little-endian. That's over-the-wire data you are
dealing with; it's *NOT* going to be host-endian. Fix the buggered
annotations instead.
No. It will break on little-endian. That's over-the-wire data you are
dealing with; it's *NOT* going to be host-endian. Fix the buggered
annotations instead.
PS: be16_to_cpu() is not the same thing as a cast. On a big-endian box,
a 16bit number 1000 (0x3e8) is stored as {3, 0xe8}; on a little-endian it's
{3, 0xe8} instead; {0xe8, 3} means 59395 there (0xe8 * 256 + 3).
be16_to_cpu() is a no-op on big-endian; on little-endian it converts the
big-endian 16bit to host-endian (internally it's a byteswap).
If over-the-wire data is stored in big-endian order (bits 8..15 in the
first byte, bits 0..7 in the second one) and you want to do any kind
of arithmetics with the value you've received, you can't blindly treat
it as u16 (unsigned short) field of structure - on big-endian boxen
it would work, but on little-endian it would give the wrong values
(59395 when sender had meant 1000). be16_to_cpu() adjusts for that.
In the above, you have ->c_npaths definitely used as a number - this
net/rds/connection.c:924: } while (++i < conn->c_npaths);
alone is enough to be convincing. All other uses are of the same
sort - it's used in comparisons, so places using it are expecting
host-endian integer.
Assignment you've modified sets it to lesser of RDS_MPATH_WORKERS (8,
apparently) and the value of buffer.rds_npaths decoded as big-endian.
"buffer" is declared as
union {
struct rds_ext_header_version version;
u16 rds_npaths;
u32 rds_gen_num;
} buffer;
and the value in it comes from
type = rds_message_next_extension(hdr, &pos, &buffer, &len);
Then we look at the return value of that rds_message_next_extension() thing
and if it's RDS_EXTHDR_NPATHS we hit that branch.
That smells like parsing the incoming package, doesn't it? A look at
rds_message_next_extension() seems to confirm that - we fetch the next
byte (that's our return value), then pick the corresponding size from
rds_exthdr_size[that_byte] and copy that many following bytes.
That code clearly expects the data to be stored in big-endian order -
it is not entirely impossible that somehow it gets host-endian (in
which case be16_to_cpu() would be a bug), but that's very unlikely.
*IF* that's over-the-wire data, the code is actually correct and the
problem is with wrong annotations -
union {
struct rds_ext_header_version version;
__be16 rds_npaths;
__be32 rds_gen_num;
} buffer;
to reflect the actual data layout to be found in there. Probably
static unsigned int rds_exthdr_size[__RDS_EXTHDR_MAX] = {
[RDS_EXTHDR_NONE] = 0,
[RDS_EXTHDR_VERSION] = sizeof(struct rds_ext_header_version),
[RDS_EXTHDR_RDMA] = sizeof(struct rds_ext_header_rdma),
[RDS_EXTHDR_RDMA_DEST] = sizeof(struct rds_ext_header_rdma_dest),
[RDS_EXTHDR_NPATHS] = sizeof(__be16),
[RDS_EXTHDR_GEN_NUM] = sizeof(__be32),
};
for consistency sake. Note that e.g. struct rds_ext_header_rdma_dest
is
struct rds_ext_header_rdma_dest {
__be32 h_rdma_rkey;
__be32 h_rdma_offset;
};
which also points towards "protocol data, fixed-endian"...
No. It will break on little-endian. That's over-the-wire data you are
dealing with; it's *NOT* going to be host-endian. Fix the buggered
annotations instead.
PS: be16_to_cpu() is not the same thing as a cast. On a big-endian box,
a 16bit number 1000 (0x3e8) is stored as {3, 0xe8}; on a little-endian it's
{3, 0xe8} instead; {0xe8, 3} means 59395 there (0xe8 * 256 + 3).
what the... You have lhash and fhash set to __be32 values, then
feed them to function that expects __be32 argument. Just turn
these two variables into __be32 and lose the pointless casts.
FWIW, here you do have a missing cast -
fhash = __ipv6_addr_jhash(faddr, rds6_hash_secret);
should be
fhash = (__force __be32)__ipv6_addr_jhash(faddr, rds6_hash_secret);
With both fhash and lhash declared as __be32.
No. It will break on little-endian. That's over-the-wire data you are
dealing with; it's *NOT* going to be host-endian. Fix the buggered
annotations instead.
PS: be16_to_cpu() is not the same thing as a cast. On a big-endian box,
a 16bit number 1000 (0x3e8) is stored as {3, 0xe8}; on a little-endian it's
{3, 0xe8} instead; {0xe8, 3} means 59395 there (0xe8 * 256 + 3).
Hi Al
This smells of an LLM generated patch. So i think you are somewhat
wasting your time explaining in detail why this is wrong. Well, maybe
in a few generations of LLM it might learn from what you said, but
that does not address the immediate problem.
We need developers using LLM to accept they have often wrong, and you
need to spend time and effort:
1) Proving it got is wrong.
2) That after a lot of effort, failing to prove it wrong, accept it might be right.
3) Proving it actually got it right.
It took me about 60 seconds to prove the POLLERR change was wrong, and
i know nothing about this code base. So it is in fact not a lot of
effort.
Andrew
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2025-08-10 19:26:19
On Sun, Aug 10, 2025 at 08:41:49PM +0200, Andrew Lunn wrote:
This smells of an LLM generated patch.
Maybe, maybe not.
So i think you are somewhat
wasting your time explaining in detail why this is wrong. Well, maybe
in a few generations of LLM it might learn from what you said, but
that does not address the immediate problem.
You do realize that there _are_ humans out there, right? Ones capable of
learning, that is...
We need developers using LLM to accept they have often wrong, and you
need to spend time and effort:
1) Proving it got is wrong.
2) That after a lot of effort, failing to prove it wrong, accept it might be right.
3) Proving it actually got it right.
It took me about 60 seconds to prove the POLLERR change was wrong, and
i know nothing about this code base. So it is in fact not a lot of
effort.
Thanks a lot for the explanation Al!
I was apprehensive about breaking things and in hindsight, should've
understood why the cast was present rather than accepting sparse's
report as the whole truth; Will go through the code more thoroughly
and send a v2 patchset.
This smells of an LLM generated patch. So i think you are somewhat
wasting your time explaining in detail why this is wrong.
I have never used (and will not use) LLMs :(
I intend to learn more about the networking stack through
contributions and I __strongly__ believe using LLMs / AI won't help me
get there.
It took me about 60 seconds to prove the POLLERR change was wrong, and
i know nothing about this code base. So it is in fact not a lot of
effort.
I looked up the definition of POLLERR on Elixir [1] and it seemed like
a valid Sparse report to me. I wasn't aware of EPOLLERR, and now
realize all the other operations are prefixed with EPOLL* in af_rds.c.
I look forward to reviews/critiques to learn from them but being
accused of using LLMs is kinda disheartening.
P.S: I'm still learning the ropes as a contributor so please pardon my
ignorance.
[1] - https://elixir.bootlin.com/linux/v6.16/source/include/uapi/asm-generic/poll.h#L9
From: Andrew Lunn <andrew@lunn.ch> Date: 2025-08-10 20:20:08
quoted
This smells of an LLM generated patch. So i think you are somewhat
wasting your time explaining in detail why this is wrong.
I have never used (and will not use) LLMs :(
Sorry, I need to refine my sense of smell.
P.S: I'm still learning the ropes as a contributor so please pardon my
ignorance.
You might find this interesting:
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
You made a few process errors as well. Since you submitted this to
net, it needs a Fixes: tag.
Also:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
Particularly:
It must either fix a real bug that bothers people
Sparse warnings don't really both people. But is the sparse warning
indicating a read bug which does bother people? If so, net is O.K, but
please add to the commit message what the real bug is. If this is
simply cleanup, not a bug fix, please submit to net-next.
You can also learn a lot by subscribing to the netdev mailing list,
and reading other peoples patches, and review comments they get.
Andrew
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2025-08-10 21:01:03
On Mon, Aug 11, 2025 at 01:01:01AM +0530, Ujwal Kundur wrote:
quoted
It took me about 60 seconds to prove the POLLERR change was wrong, and
i know nothing about this code base. So it is in fact not a lot of
effort.
I looked up the definition of POLLERR on Elixir [1] and it seemed like
a valid Sparse report to me. I wasn't aware of EPOLLERR, and now
realize all the other operations are prefixed with EPOLL* in af_rds.c.
I look forward to reviews/critiques to learn from them but being
accused of using LLMs is kinda disheartening.
As for the POLLERR part of that, the thing about POLL* constants is that
beyond the first 6 (IN/PRI/OUT/ERR/HUP/NVAL) they are arch-dependent,
and not just in a sense of bit assignments.
generic:
IN PRI OUT ERR HUP NVAL RDNORM RDBAND WRNORM WRBAND MSG REMOVE RDHUP
0 1 2 3 4 5 6 7 8 9 10 11 12
sparc:
0 1 2 3 4 5 6 7 =OUT 8 9 10 11
mips,m68k:
0 1 2 3 4 5 6 7 =OUT 8 10 11 12
xtensa:
0 1 2 3 4 5 6 7 =OUT 8 10 13 12
So these get mapped from/to by poll(2) (mangle_poll() and demangle_poll()
resp.) and __poll_t serves as a tool for catching the places that might
be confused. The internal values (also used by eventpoll(2)) are
0 1 2 3 4 5 6 7 8 9 10 --- 12
POLLREMOVE is Solaris-only thing and we do not even attempt to implement
it. So's POLLMSG, but there's a bit of a twist - nobody ever returns
that shit from ->poll(), but SIGIO from dnotify and from lease breaking
comes with ->si_band in siginfo set to POLLIN|POLLRDNORM|POLLMSG;
Warning about __poll_t is usually "mixing POLL... and EPOLL... is a bad idea".
Here it's not a bug (note that ERR gets the same bit in all of the above),
but it's trivial to annotate properly...
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2025-08-10 21:13:06
On Sun, Aug 10, 2025 at 10:00:58PM +0100, Al Viro wrote:
On Mon, Aug 11, 2025 at 01:01:01AM +0530, Ujwal Kundur wrote:
quoted
quoted
It took me about 60 seconds to prove the POLLERR change was wrong, and
i know nothing about this code base. So it is in fact not a lot of
effort.
I looked up the definition of POLLERR on Elixir [1] and it seemed like
a valid Sparse report to me. I wasn't aware of EPOLLERR, and now
realize all the other operations are prefixed with EPOLL* in af_rds.c.
I look forward to reviews/critiques to learn from them but being
accused of using LLMs is kinda disheartening.
As for the POLLERR part of that, the thing about POLL* constants is that
beyond the first 6 (IN/PRI/OUT/ERR/HUP/NVAL) they are arch-dependent,
and not just in a sense of bit assignments.