On Tue, Mar 27, 2018 at 01:39:02PM +0300, Matan Barak wrote:
> On 27/03/2018 01:58, Jason Gunthorpe wrote:
> > On Tue, Mar 20, 2018 at 03:01:36PM +0200, Leon Romanovsky wrote:
> >
> > > +static int (*flow_action_esp_keymat_validate[])(union ib_flow_action_attrs_esp_keymats *keymat) = {
> > > + [IB_UVERBS_FLOW_ACTION_ESP_KEYMAT_AES_GCM] = validate_flow_action_esp_keymat_aes_gcm,
> > > +};
> >
> > Should be
> >
> > static int (* const flow_action_esp_keymat_validate[])(union ib_flow_action_attrs_esp_keymats *keymat) = {
> >
> > To get in rodata
> >
> > Ditto for flow_action_esp_replay_validate
> >
> >
>
> Ok
>
> > > +static struct uverbs_attr_spec uverbs_flow_action_esp_keymat[] = {
> > > + [IB_UVERBS_FLOW_ACTION_ESP_KEYMAT_AES_GCM] = {
> > > + .ptr = {
> > > + .type = UVERBS_ATTR_TYPE_PTR_IN,
> > > + UVERBS_ATTR_TYPE(struct ib_uverbs_flow_action_esp_keymat_aes_gcm),
> > > + .flags = UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO,
> > > + },
> > > + },
> > > +};
> > > +
> > > +static struct uverbs_attr_spec uverbs_flow_action_esp_replay[] = {
> > > + [IB_UVERBS_FLOW_ACTION_ESP_REPLAY_BMP] = {
> > > + .ptr = {
> > > + .type = UVERBS_ATTR_TYPE_PTR_IN,
> > > + UVERBS_ATTR_STRUCT(struct ib_uverbs_flow_action_esp_replay_bmp, size),
> > > + .flags = UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO,
> > > + }
> > > + },
> > > +};
> >
> > These should be 'static const struct' too
> >
> > But that needs this change in include/rdma/uverbs_ioctl.h
> >
> > @@ -106,7 +106,7 @@ struct uverbs_attr_spec {
> > * contained in the ids array. Currently only PTR_IN
> > * attributes are supported in the ids array.
> > */
> > - struct uverbs_attr_spec *ids;
> > + const struct uverbs_attr_spec *ids;
> > } enum_def;
> > };
> > };
> >
>
> Sure
>
> > > +struct ib_uverbs_flow_action_esp_keymat_aes_gcm {
> > > + __aligned_u64 iv;
> >
> > Padding here
> > > + __u32 iv_algo; /* Use enum ib_uverbs_flow_action_esp_keymat_aes_gcm_iv_algo */
> > > +
> > > + __u32 salt;
> > > + __u32 icv_len;
> > > +
> > > + __u32 key_len;
> > > + __u32 aes_key[256 / 32];
> >
> > and implicit padding at the end here
> >
> > > +};
> >
> > And I thought we agreed to make padding explicit in this uapi
> > structures?
> >
> > Please check all the uapi structs with pahole to avoid missing
> > implicit padding.
> >
> > Thankfully the __aligned_64 makes this Not a Bug
> >
>
> Why do we need to make all uapi structs u64 aligned in our new uapi?
> We don't have the "size in quadwords" like we have in the write() path.
> Keeping them 32bit aligned with all u64 fields marked as __aligned_64 seems
> to suffice. It should prevent the nasty 32bit-vs-64bit (user-space vs
> kernel) bugs.
Alignment to 64bits makes our life much easier: less assumptions and easy reviews.
Thanks
Attachment:
signature.asc
Description: PGP signature
