* [PATCH] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow @ 2022-09-12 21:45 Nathan Huckleberry 2022-09-13 22:33 ` Nathan Chancellor 0 siblings, 1 reply; 9+ messages in thread From: Nathan Huckleberry @ 2022-09-12 21:45 UTC (permalink / raw) Cc: Nathan Huckleberry, Dan Carpenter, llvm, Greg Kroah-Hartman, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-staging, linux-kernel 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 <error27@gmail.com> Link: https://github.com/ClangBuiltLinux/linux/issues/1703 Cc: llvm@lists.linux.dev Signed-off-by: Nathan Huckleberry <nhuck@google.com> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-12 21:45 [PATCH] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow Nathan Huckleberry @ 2022-09-13 22:33 ` Nathan Chancellor 2022-09-13 22:35 ` Nathan Chancellor 0 siblings, 1 reply; 9+ messages in thread From: Nathan Chancellor @ 2022-09-13 22:33 UTC (permalink / raw) To: Nathan Huckleberry Cc: Dan Carpenter, llvm, Greg Kroah-Hartman, Nick Desaulniers, Tom Rix, linux-staging, linux-kernel 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 <error27@gmail.com> > Link: https://github.com/ClangBuiltLinux/linux/issues/1703 > Cc: llvm@lists.linux.dev > Signed-off-by: Nathan Huckleberry <nhuck@google.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-13 22:33 ` Nathan Chancellor @ 2022-09-13 22:35 ` Nathan Chancellor 2022-09-13 23:04 ` [PATCH v2] " Nathan Huckleberry 0 siblings, 1 reply; 9+ messages in thread From: Nathan Chancellor @ 2022-09-13 22:35 UTC (permalink / raw) To: Nathan Huckleberry Cc: Dan Carpenter, llvm, Greg Kroah-Hartman, Nick Desaulniers, Tom Rix, linux-staging, linux-kernel 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 <error27@gmail.com> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1703 > > Cc: llvm@lists.linux.dev > > Signed-off-by: Nathan Huckleberry <nhuck@google.com> > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> 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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-13 22:35 ` Nathan Chancellor @ 2022-09-13 23:04 ` Nathan Huckleberry 2022-09-13 23:22 ` Nathan Chancellor 0 siblings, 1 reply; 9+ messages in thread From: Nathan Huckleberry @ 2022-09-13 23:04 UTC (permalink / raw) To: nathan Cc: error27, gregkh, linux-kernel, linux-staging, llvm, ndesaulniers, nhuck, trix 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 <error27@gmail.com> Link: https://github.com/ClangBuiltLinux/linux/issues/1703 Cc: llvm@lists.linux.dev Signed-off-by: Nathan Huckleberry <nhuck@google.com> 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-13 23:04 ` [PATCH v2] " Nathan Huckleberry @ 2022-09-13 23:22 ` Nathan Chancellor 2022-09-14 21:10 ` [PATCH v3] " Nathan Huckleberry 0 siblings, 1 reply; 9+ messages in thread From: Nathan Chancellor @ 2022-09-13 23:22 UTC (permalink / raw) To: Nathan Huckleberry Cc: error27, gregkh, linux-kernel, linux-staging, llvm, ndesaulniers, trix 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 <error27@gmail.com> > Link: https://github.com/ClangBuiltLinux/linux/issues/1703 > Cc: llvm@lists.linux.dev > Signed-off-by: Nathan Huckleberry <nhuck@google.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-13 23:22 ` Nathan Chancellor @ 2022-09-14 21:10 ` Nathan Huckleberry 2022-09-15 8:21 ` Arnd Bergmann 0 siblings, 1 reply; 9+ messages in thread From: Nathan Huckleberry @ 2022-09-14 21:10 UTC (permalink / raw) To: nathan Cc: error27, gregkh, linux-kernel, linux-staging, llvm, ndesaulniers, nhuck, trix 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 <error27@gmail.com> Link: https://github.com/ClangBuiltLinux/linux/issues/1703 Cc: llvm@lists.linux.dev Signed-off-by: Nathan Huckleberry <nhuck@google.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-14 21:10 ` [PATCH v3] " Nathan Huckleberry @ 2022-09-15 8:21 ` Arnd Bergmann 2022-09-15 9:09 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Arnd Bergmann @ 2022-09-15 8:21 UTC (permalink / raw) To: Nathan Huckleberry, nathan Cc: error27, Greg Kroah-Hartman, linux-kernel, linux-staging, llvm, Nick Desaulniers, trix 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 <error27@gmail.com> > Link: https://github.com/ClangBuiltLinux/linux/issues/1703 > Cc: llvm@lists.linux.dev > Signed-off-by: Nathan Huckleberry <nhuck@google.com> > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > --- > > 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 <arnd@arndb.de> 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-15 8:21 ` Arnd Bergmann @ 2022-09-15 9:09 ` Dan Carpenter 2022-09-15 12:03 ` Arnd Bergmann 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2022-09-15 9:09 UTC (permalink / raw) To: Arnd Bergmann Cc: Nathan Huckleberry, nathan, error27, Greg Kroah-Hartman, linux-kernel, linux-staging, llvm, Nick Desaulniers, trix 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 <error27@gmail.com> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1703 > > Cc: llvm@lists.linux.dev > > Signed-off-by: Nathan Huckleberry <nhuck@google.com> > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > > > --- > > > > 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 <arnd@arndb.de> > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow 2022-09-15 9:09 ` Dan Carpenter @ 2022-09-15 12:03 ` Arnd Bergmann 0 siblings, 0 replies; 9+ messages in thread From: Arnd Bergmann @ 2022-09-15 12:03 UTC (permalink / raw) To: Dan Carpenter Cc: Nathan Huckleberry, nathan, error27, Greg Kroah-Hartman, linux-kernel, linux-staging, llvm, Nick Desaulniers, trix 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] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-15 12:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-12 21:45 [PATCH] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow Nathan Huckleberry 2022-09-13 22:33 ` Nathan Chancellor 2022-09-13 22:35 ` Nathan Chancellor 2022-09-13 23:04 ` [PATCH v2] " Nathan Huckleberry 2022-09-13 23:22 ` Nathan Chancellor 2022-09-14 21:10 ` [PATCH v3] " Nathan Huckleberry 2022-09-15 8:21 ` Arnd Bergmann 2022-09-15 9:09 ` Dan Carpenter 2022-09-15 12:03 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).