Date:   Fri, 17 Aug 2018 16:43:20 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Ganesh Goudar <ganeshgr@...lsio.com>
Cc:     Rahul Lakkireddy <rahul.lakkireddy@...lsio.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [endianness bug] cxgb4: mk_act_open_req() buggers
 ->{local,peer}_ip on big-endian hosts

On Fri, Aug 17, 2018 at 06:35:41PM +0530, Ganesh Goudar wrote:
> Thanks, Al. The patch looks good to me but it does not seem
> to be showing up in patchwork, should I resend the patch on
> your behalf to net tree ?

Umm...  I thought net-next had been closed until -rc1, hadn't
it?

Anyway, endianness cleanups and fixes of drivers/net/ethernet/chelsio
can be found in vfs.git #net-endian.chelsio; I was planning to post
that stuff on netdev after -rc1, but I would certainly appreciate
a look from somebody familiar with the hardware prior to that, assuming
you have time for that at the moment...  The stuff in there (it's
based off net/master):
      struct cxgb4_next_header .match_val/.match_mask/mask should be net-endian
      cxgb4: fix TC-PEDIT-related parts of cxgb4_tc_flower on big-endian
      cxgb4_tc_u32: trivial endianness annotations
      cxgb4: trivial endianness annotations
      libcxgb: trivial __percpu annotations
      libcxgb: trivial endianness annotations
      cxgb3: trivial endianness annotations
      cxgb3: don't get cute with !! and shifts in t3_prep_adapter()...
      [investigate][endianness bug] cxgb3: assigning htonl(11-bit value) to __be16 field is wrong
      cxgb: trivial endianness annotations

The first two are fixes (the second being the patch you've just replied to),
next-to-last might or might not be (see "[RFC] weirdness in
cxgb3_main.c:init_tp_parity()" posted to netdev a couple of weeks
ago), the rest is pure annotations.  Result is not entirely sparse-clean, but
fairly close to that:

drivers/net/ethernet/chelsio/cxgb3/t3_hw.c:681:67: warning: incorrect type in argument 3 (different base types)
drivers/net/ethernet/chelsio/cxgb3/t3_hw.c:681:67:    expected restricted __le32 [usertype] data
drivers/net/ethernet/chelsio/cxgb3/t3_hw.c:681:67:    got int
drivers/net/ethernet/chelsio/cxgb3/t3_hw.c:898:31: warning: incorrect type in assignment (different base types)
drivers/net/ethernet/chelsio/cxgb3/t3_hw.c:898:31:    expected unsigned int [unsigned] [usertype] <noident>
drivers/net/ethernet/chelsio/cxgb3/t3_hw.c:898:31:    got restricted __be32 [usertype] <noident>
drivers/net/ethernet/chelsio/cxgb3/sge.c:2435:43: warning: incorrect type in assignment (different base types)
drivers/net/ethernet/chelsio/cxgb3/sge.c:2435:43:    expected restricted __wsum [usertype] csum
drivers/net/ethernet/chelsio/cxgb3/sge.c:2435:43:    got restricted __be32 [assigned] [usertype] rss_hi
drivers/net/ethernet/chelsio/cxgb3/sge.c:2436:47: warning: incorrect type in assignment (different base types)
drivers/net/ethernet/chelsio/cxgb3/sge.c:2436:47:    expected unsigned int [unsigned] [usertype] priority
drivers/net/ethernet/chelsio/cxgb3/sge.c:2436:47:    got restricted __be32 [assigned] [usertype] rss_lo
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:419:38: warning: incorrect type in assignment (different base types)
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:419:38:    expected restricted __be32 [usertype] mask
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:419:38:    got unsigned int
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:420:37: warning: incorrect type in assignment (different base types)
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:420:37:    expected restricted __be32 [usertype] val
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:420:37:    got unsigned int
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:449:22: warning: incorrect type in assignment (different base types)
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:449:22:    expected restricted __be32 [usertype] mask
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c:449:22:    got unsigned int

Remaining tc_flower warnings are misannotated tcf_pedit_mask()/tcf_pedit_val()
(both are actually returning __be32)

sge.c ones are
                                /* Preserve the RSS info in csum & priority */
                                skb->csum = rss_hi;
                                skb->priority = rss_lo;
and I've no idea whether it's a problem or not - ->csum is almost
certainly being abused there, and it looks like ->priority also
is...  I don't know - it's certainly used as host-endian in the
same driver (see e.g. queue_set() and is_ctrl_pkt()), but those are
on TX path and this is RX...  I'm not familiar enough with the
code to tell what's going on here - what *is* the consumer of
the data stored there?  cxgb3_offload.c get_hwtid() and get_opcode()?
If so (and if that's all there is), why not simply something like
	skb->priority = (ntohl(rss_lo) & 0xffff00) | G_OPCODE(ntohl(rss_hi));
before passing skb to rx_offload(), with
static inline u32 get_hwtid(struct sk_buff *skb)
{
        return skb->priority >> 8;
}
static inline u32 get_opcode(struct sk_buff *skb)
{
        return skb->priority & 0xff;
}
and don't bother touching ->csum...  Again, I'm not familiar with hardware
*or* the driver; I've no idea how much is e.g. shared with the infiniband
or scsi sides of the things (hadn't even looked there).  So this one is
up to somebody familiar with the design of that thing.

t3_hw.c: the first one is
        return t3_seeprom_write(adapter, EEPROM_STAT_ADDR, enable ? 0xc : 0);
which looks bloody odd, seeing that t3_seeprom_write() expects __le32 as
the last argument and appears to be serious about that -
        pci_write_config_dword(adapter->pdev, base + PCI_VPD_DATA,
                               le32_to_cpu(data));
is where that 0 or 0xc ends up.  Might or might not be a bug, but I'd
suggest triggering that sucker on big-endian host and checking whether
it works there...

The second in t3_hw.c...  I'd be tempted to make sf1_read() to do
htonl() before storing the result, adjusted the callers (i.e.
                if (!(status & htonl(1)))
in flash_wait_op(), no byteswaps in t3_read_flash()) and
had t3_read_flash() do explicit ntohl() on the value read before
storing it in *vers, but that's cosmetical anyway - making it
easier for sparse (and human readers) to keep track of what's
host- and what's net-endian; no real bug there.

Said that, there's another thing worth looking into - uses of __force
in the driver(s).  I mean, look at
        union {
                u32 word;
                char byte[4];
        } last;
        unsigned char *bp;
        int i;

        if (dir == T4_MEMORY_READ) {
                last.word = le32_to_cpu((__force __le32)
                                        t4_read_reg(adap, addr));
                for (bp = (unsigned char *)buf, i = off; i < 4; i++)
                        bp[i] = last.byte[i];

That "__force __le32" crap is a plain and simple result of confusion -
the code clearly intends last.word to be fixed-endian, since it then
proceeds to access individual bytes via aliasing.  IOW, this
le32_to_cpu() is a misspelled cpu_to_le32(), and the force-cast
is an attempt to confuse sparse into not noticing the misannotation.
All too successful attempt, at that - the other half
        } else {
                last.word = *buf;
                for (i = off; i < 4; i++)
                        last.byte[i] = 0;
                t4_write_reg(adap, addr,
                             (__force u32)cpu_to_le32(last.word));
        }
also wants last.word fixed-endian, which makes the first assignment
very dubious - buf is a pointer to _byte_, so that makes very little
sense.  I very much doubt that this thing (t4_memory_rw_residual())
is correct - e.g. if len = 4n + 2 in "write" t4_memory_rw(), the
first 4n bytes will be fed to hardware and then
	* on little-endian hbuf[4n] will be padded with 3 zeroes and
passed to t4_write_reg(), completely ignoring hbuf[4n+1], while
	* on big-endian zero will be passed to t4_write_reg(),
completely ignoring both hbuf[4n] and hbuf[4n+1].
Looks rather odd, doesn't it?

It smells like
	__le32 v = 0;
	memcpy(&v, buf, off);
	t4_write_reg(adap, addr, le32_to_cpu(v));
on the write side.  And on the read side...  Are you sure you want
to copy 4 - off bytes?  After all, if it was a *read* for 4n+1 bytes,
we'll copy the first 4n bytes from the hardware, then call that thing
with off = 1.  Presumably wanting to copy one more byte, not three...
Should that be
	__le32 v = cpu_to_le32(t4_read_reg(adap, addr));
	memcpy(buf, &v, off);
instead?

Looking at the callers, I'd say that t4_memory_rw() ought to declare
buf as __le32 *, and have
                if (dir == T4_MEMORY_READ)
                        *buf++ = cpu_to_le32(t4_read_reg(adap,
                                                mem_base + offset));
                else
                        t4_write_reg(adap, mem_base + offset,
                                     le32_to_cpu(*buf++));
                offset += sizeof(__le32);
                len -= sizeof(__le32);
and to hell with force-casts in there (BTW, where has __be32 come
from in the mainline?  It's only in sizeof, but still...).

And cudbg_memory_read() looks somewhat fishy in one more way -
you are doing 64bit host memory stores to pointer that is only
32bit-aligned.  Sure, x86 can cope, but what about e.g. sparc64?
And what of the PCIe side of that - what happens if your 64bit
read straddles the aperture window boundary?  You only check that
addr is 32bit-aligned, so what happens if you set it 4 bytes
below the window end and ask to read you 8 bytes?

Powered by blists - more mailing lists