On Tue, Sep 13, 2022 at 03:33:24PM -0700, Nathan Chancellor wrote:
> On Mon, Sep 12, 2022 at 02:45:20PM -0700, Nathan Huckleberry wrote:
> > The ndo_start_xmit field in net_device_ops is expected to be of type
> > netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev).
> >
> > The mismatched return type breaks forward edge kCFI since the underlying
> > function definition does not match the function hook definition.
> >
> > The return type of cvm_oct_xmit and cvm_oct_xmit_pow should be changed
> > from int to netdev_tx_t.
> >
> > Reported-by: Dan Carpenter <[email protected]>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1703
> > Cc: [email protected]
> > Signed-off-by: Nathan Huckleberry <[email protected]>
>
> Reviewed-by: Nathan Chancellor <[email protected]>
Sorry, forgot to point out that the prototypes in
drivers/staging/octeon/ethernet-tx.h should be updated as well.
> > ---
> > drivers/staging/octeon/ethernet-tx.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
> > index 1ad94c5060b5..a36e36701c74 100644
> > --- a/drivers/staging/octeon/ethernet-tx.c
> > +++ b/drivers/staging/octeon/ethernet-tx.c
> > @@ -125,7 +125,7 @@ static void cvm_oct_free_tx_skbs(struct net_device *dev)
> > *
> > * Returns Always returns NETDEV_TX_OK
> > */
> > -int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
> > +netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
> > {
> > union cvmx_pko_command_word0 pko_command;
> > union cvmx_buf_ptr hw_buffer;
> > @@ -506,7 +506,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
> > * @dev: Device info structure
> > * Returns Always returns zero
> > */
> > -int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
> > +netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
> > {
> > struct octeon_ethernet *priv = netdev_priv(dev);
> > void *packet_buffer;
> > --
> > 2.37.2.789.g6183377224-goog
> >
>
The ndo_start_xmit field in net_device_ops is expected to be of type
netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev).
The mismatched return type breaks forward edge kCFI since the underlying
function definition does not match the function hook definition.
The return type of cvm_oct_xmit and cvm_oct_xmit_pow should be changed
from int to netdev_tx_t.
Reported-by: Dan Carpenter <[email protected]>
Link: https://github.com/ClangBuiltLinux/linux/issues/1703
Cc: [email protected]
Signed-off-by: Nathan Huckleberry <[email protected]>
Changes v1 -> v2:
- Update function signatures in ethernet-tx.h.
---
drivers/staging/octeon/ethernet-tx.c | 4 ++--
drivers/staging/octeon/ethernet-tx.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index 1ad94c5060b5..a36e36701c74 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -125,7 +125,7 @@ static void cvm_oct_free_tx_skbs(struct net_device *dev)
*
* Returns Always returns NETDEV_TX_OK
*/
-int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
+netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
{
union cvmx_pko_command_word0 pko_command;
union cvmx_buf_ptr hw_buffer;
@@ -506,7 +506,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
* @dev: Device info structure
* Returns Always returns zero
*/
-int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
+netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
{
struct octeon_ethernet *priv = netdev_priv(dev);
void *packet_buffer;
diff --git a/drivers/staging/octeon/ethernet-tx.h b/drivers/staging/octeon/ethernet-tx.h
index 78936e9b33b0..6c524668f65a 100644
--- a/drivers/staging/octeon/ethernet-tx.h
+++ b/drivers/staging/octeon/ethernet-tx.h
@@ -5,8 +5,8 @@
* Copyright (c) 2003-2007 Cavium Networks
*/
-int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
-int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
+netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
+netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
int cvm_oct_transmit_qos(struct net_device *dev, void *work_queue_entry,
int do_free, int qos);
void cvm_oct_tx_initialize(void);
--
2.37.2.789.g6183377224-goog
On Tue, Sep 13, 2022 at 04:04:12PM -0700, Nathan Huckleberry wrote:
> The ndo_start_xmit field in net_device_ops is expected to be of type
> netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev).
>
> The mismatched return type breaks forward edge kCFI since the underlying
> function definition does not match the function hook definition.
>
> The return type of cvm_oct_xmit and cvm_oct_xmit_pow should be changed
> from int to netdev_tx_t.
>
> Reported-by: Dan Carpenter <[email protected]>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1703
> Cc: [email protected]
> Signed-off-by: Nathan Huckleberry <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
>
> Changes v1 -> v2:
> - Update function signatures in ethernet-tx.h.
>
> ---
> drivers/staging/octeon/ethernet-tx.c | 4 ++--
> drivers/staging/octeon/ethernet-tx.h | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
> index 1ad94c5060b5..a36e36701c74 100644
> --- a/drivers/staging/octeon/ethernet-tx.c
> +++ b/drivers/staging/octeon/ethernet-tx.c
> @@ -125,7 +125,7 @@ static void cvm_oct_free_tx_skbs(struct net_device *dev)
> *
> * Returns Always returns NETDEV_TX_OK
> */
> -int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
> +netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> union cvmx_pko_command_word0 pko_command;
> union cvmx_buf_ptr hw_buffer;
> @@ -506,7 +506,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
> * @dev: Device info structure
> * Returns Always returns zero
> */
> -int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
> +netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
> {
> struct octeon_ethernet *priv = netdev_priv(dev);
> void *packet_buffer;
> diff --git a/drivers/staging/octeon/ethernet-tx.h b/drivers/staging/octeon/ethernet-tx.h
> index 78936e9b33b0..6c524668f65a 100644
> --- a/drivers/staging/octeon/ethernet-tx.h
> +++ b/drivers/staging/octeon/ethernet-tx.h
> @@ -5,8 +5,8 @@
> * Copyright (c) 2003-2007 Cavium Networks
> */
>
> -int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
> -int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
> +netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
> +netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
> int cvm_oct_transmit_qos(struct net_device *dev, void *work_queue_entry,
> int do_free, int qos);
> void cvm_oct_tx_initialize(void);
> --
> 2.37.2.789.g6183377224-goog
>
The ndo_start_xmit field in net_device_ops is expected to be of type
netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev).
The mismatched return type breaks forward edge kCFI since the underlying
function definition does not match the function hook definition.
The return type of cvm_oct_xmit and cvm_oct_xmit_pow should be changed
from int to netdev_tx_t.
Reported-by: Dan Carpenter <[email protected]>
Link: https://github.com/ClangBuiltLinux/linux/issues/1703
Cc: [email protected]
Signed-off-by: Nathan Huckleberry <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
---
Changes v1 -> v2:
- Update function signatures in ethernet-tx.h.
Changes v2 -> v3:
- Move changes below the scissors --- so they don't show in commit msg
- Add reviewed-by tag
drivers/staging/octeon/ethernet-tx.c | 4 ++--
drivers/staging/octeon/ethernet-tx.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index 1ad94c5060b5..a36e36701c74 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -125,7 +125,7 @@ static void cvm_oct_free_tx_skbs(struct net_device *dev)
*
* Returns Always returns NETDEV_TX_OK
*/
-int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
+netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
{
union cvmx_pko_command_word0 pko_command;
union cvmx_buf_ptr hw_buffer;
@@ -506,7 +506,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
* @dev: Device info structure
* Returns Always returns zero
*/
-int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
+netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
{
struct octeon_ethernet *priv = netdev_priv(dev);
void *packet_buffer;
diff --git a/drivers/staging/octeon/ethernet-tx.h b/drivers/staging/octeon/ethernet-tx.h
index 78936e9b33b0..6c524668f65a 100644
--- a/drivers/staging/octeon/ethernet-tx.h
+++ b/drivers/staging/octeon/ethernet-tx.h
@@ -5,8 +5,8 @@
* Copyright (c) 2003-2007 Cavium Networks
*/
-int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
-int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
+netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
+netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
int cvm_oct_transmit_qos(struct net_device *dev, void *work_queue_entry,
int do_free, int qos);
void cvm_oct_tx_initialize(void);
--
2.37.2.789.g6183377224-goog
On Wed, Sep 14, 2022, at 11:10 PM, Nathan Huckleberry wrote:
> The ndo_start_xmit field in net_device_ops is expected to be of type
> netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev).
>
> The mismatched return type breaks forward edge kCFI since the underlying
> function definition does not match the function hook definition.
>
> The return type of cvm_oct_xmit and cvm_oct_xmit_pow should be changed
> from int to netdev_tx_t.
>
> Reported-by: Dan Carpenter <[email protected]>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1703
> Cc: [email protected]
> Signed-off-by: Nathan Huckleberry <[email protected]>
> Reviewed-by: Nathan Chancellor <[email protected]>
>
> ---
>
> Changes v1 -> v2:
> - Update function signatures in ethernet-tx.h.
>
> Changes v2 -> v3:
> - Move changes below the scissors --- so they don't show in commit msg
> - Add reviewed-by tag
The patch looks correct to me so
Acked-by: Arnd Bergmann <[email protected]>
but I have two more general comments:
- For your changelogs, it would help to include the diagnostic message
from smatch that you link to.
- 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.
Arnd
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:
> > The ndo_start_xmit field in net_device_ops is expected to be of type
> > netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, struct net_device *dev).
> >
> > The mismatched return type breaks forward edge kCFI since the underlying
> > function definition does not match the function hook definition.
> >
> > The return type of cvm_oct_xmit and cvm_oct_xmit_pow should be changed
> > from int to netdev_tx_t.
> >
> > Reported-by: Dan Carpenter <[email protected]>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1703
> > Cc: [email protected]
> > Signed-off-by: Nathan Huckleberry <[email protected]>
> > Reviewed-by: Nathan Chancellor <[email protected]>
> >
> > ---
> >
> > Changes v1 -> v2:
> > - Update function signatures in ethernet-tx.h.
> >
> > Changes v2 -> v3:
> > - Move changes below the scissors --- so they don't show in commit msg
> > - Add reviewed-by tag
>
> The patch looks correct to me so
>
> Acked-by: Arnd Bergmann <[email protected]>
>
> but I have two more general comments:
>
> - For your changelogs, it would help to include the diagnostic message
> from smatch that you link to.
>
> - 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.
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.
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.
Sparse and thus Smatch default to unsigned int as well, but they won't
make the enum smaller for a struct. There are some implications of
such as can an enum be less than zero? If no then there is a potential
for if (err < 0) being a bug, and if yes then there is a potential for
array underflows
regards,
dan carpenter
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]