Re: [PATCH v3] staging: octeon: Fix return type of cvm_oct_xmit and cvm_oct_xmit_pow

[Dan Carpenter]

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@xxxxxxxxx>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1703
> > Cc: llvm@xxxxxxxxxxxxxxx
> > Signed-off-by: Nathan Huckleberry <nhuck@xxxxxxxxxx>
> > Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> >
> > ---
> >
> > 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@xxxxxxxx>
> 
> 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