Re: [PATCH rdma-next 05/15] IB/uverbs: Add flow_action create and destroy verbs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 27/03/2018 15:38, Leon Romanovsky wrote:
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.


If alignments were implicit - sure.
But since they're currently explicit (adding reserved fields), it happened more than once that developers forgot "reserved" fields, breaking 32bit vs 64bit.

Thanks


Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux