Message-ID: <20250810182506.GL222315@ZenIV>
Date: Sun, 10 Aug 2025 19:25:06 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Ujwal Kundur <ujwal.kundur@...il.com>
Cc: allison.henderson@...cle.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
	netdev@...r.kernel.org, linux-rdma@...r.kernel.org,
	rds-devel@....oracle.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] rds: Fix endian annotations across various
 assignments

On Sun, Aug 10, 2025 at 06:47:05PM +0100, Al Viro wrote:

> >  		switch (type) {
> >  		case RDS_EXTHDR_NPATHS:
> >  			conn->c_npaths = min_t(int, RDS_MPATH_WORKERS,
> > -					       be16_to_cpu(buffer.rds_npaths));
> > +					      (__force __u16)buffer.rds_npaths);
> 
> 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"...

Powered by blists - more mailing lists