Date:   Thu, 15 Sep 2022 14:03:12 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Dan Carpenter" <dan.carpenter@...cle.com>
Cc:     "Nathan Huckleberry" <nhuck@...gle.com>, nathan@...nel.org,
        error27@...il.com,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev,
        llvm@...ts.linux.dev, "Nick Desaulniers" <ndesaulniers@...gle.com>,
        trix@...hat.com
Subject: Re: [PATCH v3] staging: octeon: Fix return type of cvm_oct_xmit and
 cvm_oct_xmit_pow

On Thu, Sep 15, 2022, at 11:09 AM, Dan Carpenter wrote:
> On Thu, Sep 15, 2022 at 10:21:47AM +0200, Arnd Bergmann wrote:
>> On Wed, Sep 14, 2022, at 11:10 PM, Nathan Huckleberry wrote:
> > - This has probably been discussed before, but why is this only
> >   reported by smatch but by clang itself when building with CFI
> >   enabled? It appears that CFI enforces stricter C++ style type
> >   compatibility on enums while the warnings only catch incompatible
> >   types according to the normal C11 rules.
>
> This is not in a released version of Smatch.  I wrote the check and
> attached it to the email with the bug reports but I wasn't really sure
> how enums are handled in Clang. It's a gray area in the C standard.
>
> I'll release it now since no one complained about false positives, but
> yes, ideally this would be built into the compiler.

I think there are two separate issues here:

- clang-cfi being inconsistent regarding which types it considers
  compatible, compared to what code it otherwise sees as valid.

- generally warning about valid conversions between integer pointers
  and pointers to compatible enums, along the lines of -Wpointer-sign
  and -Wenum-enum-conversion.

> GCC does some sort of surprising things with enums and the kernel relies
> on it in various places.  By default enums in GCC are unsigned int.  If
> they have to store values which don't fit into unsigned int (negatives
> or larger than UINT_MAX) type is adjusted to be signed or a larger type.

Isn't this defined in the ELF psABI for a given architecture, rather
than compiler specific or in the C standard? E.g. the Arm AAPCS
document lists

8.1.3 Enumerated Types
This ABI delegates a choice of representation of enumerated types to
a platform ABI (whether defined by a standardor by custom and practice)
or to an interface contract if there is no defined platform ABI.
The two permitted ABI variants are:
• An enumerated type normally occupies a word (int or unsigned int).
  If a word cannot represent all of its enumerated values the type
  occupies a double word (long long or unsigned long long).
• The type of the storage container for an enumerated type is the
  smallest integer type that can contain all of its enumerated 
  values. When both the signed and unsigned versions of an integer
  type can represent all values, this ABI recommends that the
  unsigned type should be preferred (in line with common practice).

> Also if the enum is in a struct and the type can be made smaller then
> GCC will.  And example of this is in union myrs_cmd_mbox where the
> enum myrs_cmd_opcode opcode only takes one byte. The SCSI_MYRB driver
> relies on the struct thing and will corrupt memory if the struct is
> larger than expected.  This is the only example I know of in the kernel
> where this matters.

Ah, interesting, I had no idea. Experimenting with it a bit
showed this to be independent of being in a struct, and only
a feature of GCC's "packed" attribute. E.g. this line creates
a zero-initialized single byte variable and an overflow warning:

enum { A } __attribute__((packed)) e = 256;

There is also a type mismatch warning with both gcc and clang
when trying to use function types that differ in the size or
signedness of an enum:

typedef enum { A } __attribute__((packed)) byteenum;
extern unsigned int f(void);
byteenum (*fp)(void) = f;

For the case of netdev_tx_t, we can actually take advantage of this
with a patch to make it an incompatible length to create a compiler
warning. See patch and output below.

    Arnd

--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -117,12 +117,11 @@ void netdev_set_default_ethtool_ops(struct net_device *dev,
 /* Driver transmit return codes */
 #define NETDEV_TX_MASK         0xf0
 
-enum netdev_tx {
-       __NETDEV_TX_MIN  = INT_MIN,     /* make sure enum is signed */
+typedef enum {
+       __NETDEV_TX_MIN  = S8_MIN,      /* make sure enum is signed */
        NETDEV_TX_OK     = 0x00,        /* driver took care of packet */
        NETDEV_TX_BUSY   = 0x10,        /* driver tx path was busy*/
-};
-typedef enum netdev_tx netdev_tx_t;
+} __packed netdev_tx_t;
 
 /*
  * Current order:

ethernet/broadcom/bcm4908_enet.c:677:27: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
bond_alb.h:160:5: note: previous declaration of 'bond_tlb_xmit' with type 'int(struct sk_buff *, struct net_device *)'
bond_alb.h:159:5: note: previous declaration of 'bond_alb_xmit' with type 'int(struct sk_buff *, struct net_device *)'
ethernet/litex/litex_liteeth.c:199:35: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
hamradio/baycom_epp.c:1119:32: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/asix/ax88796c_main.c:937:35: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
wwan/t7xx/t7xx_netdev.c:111:29: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/microchip/sparx5/sparx5_netdev.c:223:35: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/sunplus/spl2sw_driver.c:198:27: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/korina.c:1272:35: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/sfc/ef100_tx.h:25:13: note: previous declaration of 'ef100_enqueue_skb' with type 'netdev_tx_t(struct efx_tx_queue *, struct sk_buff *)'
ethernet/microchip/lan966x/lan966x_main.c:461:43: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/ti/davinci_emac.c:1718:35: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/davicom/dm9000.c:1372:35: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]
ethernet/ti/netcp_core.c:1944:35: error: initialization of 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' from incompatible pointer type 'int (*)(struct sk_buff *, struct net_device *)' [-Werror=incompatible-pointer-types]

Powered by blists - more mailing lists