smatch/sparse complaints on static assertion

smatch/sparse complaints on static assertion

[Shannon Nelson]

Hi All,

I'm getting complaints from smatch on the ionic network driver's static 
assertions and am not sure why it was complaining.  Dan Carpenter 
suggested this might be an issue in sparse with how it is calculating 
the sizes of the unions.

I ran this at the top of a pretty recent net-next tree 
(v5.5-rc7-1839-g8192c36)
$ ../smatch/smatch_scripts/kchecker drivers/net/ethernet/pensando/ionic/

And got several copies of this:

drivers/net/ethernet/pensando/ionic/ionic_dev.h:38:1: error: static 
assertion failed: "sizeof(union ionic_dev_regs) == 4096"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:40:1: error: static 
assertion failed: "sizeof(union ionic_dev_cmd_regs) == 2048"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:56:1: error: static 
assertion failed: "sizeof(struct ionic_dev_getattr_comp) == 16"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:57:1: error: static 
assertion failed: "sizeof(struct ionic_dev_setattr_cmd) == 64"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:58:1: error: static 
assertion failed: "sizeof(struct ionic_dev_setattr_comp) == 16"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:68:1: error: static 
assertion failed: "sizeof(struct ionic_port_getattr_comp) == 16"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:78:1: error: static 
assertion failed: "sizeof(struct ionic_lif_getattr_comp) == 16"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:79:1: error: static 
assertion failed: "sizeof(struct ionic_lif_setattr_cmd) == 64"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:80:1: error: static 
assertion failed: "sizeof(struct ionic_lif_setattr_comp) == 16"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:82:1: error: static 
assertion failed: "sizeof(struct ionic_q_init_cmd) == 64"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:117:1: error: static 
assertion failed: "sizeof(struct ionic_vf_setattr_cmd) == 64"
drivers/net/ethernet/pensando/ionic/ionic_dev.h:120:1: error: static 
assertion failed: "sizeof(struct ionic_vf_getattr_comp) == 16"

These static assertion lines have been fine up until now and I'm pretty 
sure they are correct.

Has this issue been seen elsewhere?  Or is there something I can do in 
our code to get rid of the complaints?

Thanks,
sln

Re: smatch/sparse complaints on static assertion

[Luc Van Oostenryck]

On Tue, Feb 11, 2020 at 09:41:05AM -0800, Shannon Nelson wrote:
> Hi All,
> 
> I'm getting complaints from smatch on the ionic network driver's static
> assertions and am not sure why it was complaining.  Dan Carpenter suggested
> this might be an issue in sparse with how it is calculating the sizes of the
> unions.
> 
> I ran this at the top of a pretty recent net-next tree
> (v5.5-rc7-1839-g8192c36)
> $ ../smatch/smatch_scripts/kchecker drivers/net/ethernet/pensando/ionic/
> 
> And got several copies of this:
> 
> drivers/net/ethernet/pensando/ionic/ionic_dev.h:38:1: error: static
> assertion failed: "sizeof(union ionic_dev_regs) == 4096"

...
 
> These static assertion lines have been fine up until now and I'm pretty sure
> they are correct.
> 
> Has this issue been seen elsewhere?  Or is there something I can do in our
> code to get rid of the complaints?

This is caused by the packing of the structs. It's using
	#pragma pack(push, 1) / #pragma pack(pop)
which is not supported by Sparse. Packing via __attribute__((packed))
is incomplete but the pragmas are currently completly ignored.

-- Luc Van Oostenryck

Re: smatch/sparse complaints on static assertion

[Linus Torvalds]

On Tue, Feb 11, 2020 at 9:41 AM Shannon Nelson <snelson@pensando.io> wrote:
>
> drivers/net/ethernet/pensando/ionic/ionic_dev.h:56:1: error: static
> assertion failed: "sizeof(struct ionic_dev_getattr_comp) == 16"

As Luc says, this is because those structures are mis-declared.

See this, for example:

  struct ionic_dev_getattr_comp {
        u8     status;
        u8     rsvd[3];
        union {
                __le64  features;
                u8      rsvd2[11];
        };
        u8     color;
  };

and notice how "__le64  features" is a 64-bit entity but it's in a
union with a "u8 rsvd2[11];".

That makes the whole union align to the same as the __le64 (on x86-32,
that's 32-bit, for bad legacy reasons, on everything else it's
64-bit).

Mark the associated types properly packed individually, rather than
use the disgusting "pragma pack()" that should never be used.

This is not a recent sparse change, it must never have worked.

            Linus

Re: smatch/sparse complaints on static assertion

[Shannon Nelson]

On 2/11/20 4:31 PM, Luc Van Oostenryck wrote:
> On Tue, Feb 11, 2020 at 09:41:05AM -0800, Shannon Nelson wrote:
>> Hi All,
>>
>> I'm getting complaints from smatch on the ionic network driver's static
>> assertions and am not sure why it was complaining.  Dan Carpenter suggested
>> this might be an issue in sparse with how it is calculating the sizes of the
>> unions.
>>
>> I ran this at the top of a pretty recent net-next tree
>> (v5.5-rc7-1839-g8192c36)
>> $ ../smatch/smatch_scripts/kchecker drivers/net/ethernet/pensando/ionic/
>>
>> And got several copies of this:
>>
>> drivers/net/ethernet/pensando/ionic/ionic_dev.h:38:1: error: static
>> assertion failed: "sizeof(union ionic_dev_regs) == 4096"
> ...
>   
>> These static assertion lines have been fine up until now and I'm pretty sure
>> they are correct.
>>
>> Has this issue been seen elsewhere?  Or is there something I can do in our
>> code to get rid of the complaints?
> This is caused by the packing of the structs. It's using
> 	#pragma pack(push, 1) / #pragma pack(pop)
> which is not supported by Sparse. Packing via __attribute__((packed))
> is incomplete but the pragmas are currently completly ignored.
>
> -- Luc Van Oostenryck

Ah, that makes sense.  Thanks.
sln

Re: smatch/sparse complaints on static assertion

[Shannon Nelson]

On 2/11/20 4:36 PM, Linus Torvalds wrote:
> On Tue, Feb 11, 2020 at 9:41 AM Shannon Nelson <snelson@pensando.io> wrote:
>> drivers/net/ethernet/pensando/ionic/ionic_dev.h:56:1: error: static
>> assertion failed: "sizeof(struct ionic_dev_getattr_comp) == 16"
> As Luc says, this is because those structures are mis-declared.
>
> See this, for example:
>
>    struct ionic_dev_getattr_comp {
>          u8     status;
>          u8     rsvd[3];
>          union {
>                  __le64  features;
>                  u8      rsvd2[11];
>          };
>          u8     color;
>    };
>
> and notice how "__le64  features" is a 64-bit entity but it's in a
> union with a "u8 rsvd2[11];".
>
> That makes the whole union align to the same as the __le64 (on x86-32,
> that's 32-bit, for bad legacy reasons, on everything else it's
> 64-bit).
>
> Mark the associated types properly packed individually, rather than
> use the disgusting "pragma pack()" that should never be used.
>
> This is not a recent sparse change, it must never have worked.
>
>              Linus

Thanks, I'll try to work that into this next net-next cycle.

sln