RE: [RFC][PATCH 03/12] digest_lists: Basic definitions

From: Greg KH [mailto:gregkh@linuxfoundation.org]
Sent: Sunday, June 27, 2021 12:54 PM
On Fri, Jun 25, 2021 at 06:56:05PM +0200, Roberto Sassu wrote:

quoted

--- /dev/null
+++ b/include/uapi/linux/digest_lists.h

@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) 2017-2021 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.

As you already have the SPDX line up there, you do not need this
paragraph.  Please remove it from all of the new files you have added in
this series.

Ok.

quoted

+ *
+ * File: digest_lists.h

We know the filename, no need to have it here again.

quoted

+ *      Digest list definitions exported to user space.

Now this is what probably needs more information...

Ok. Yes, these definitions are useful to generate digest lists
in user space.

quoted

+ */
+
+#ifndef _UAPI__LINUX_DIGEST_LISTS_H
+#define _UAPI__LINUX_DIGEST_LISTS_H
+
+#include <linux/types.h>
+#include <linux/hash_info.h>
+
+enum compact_types { COMPACT_KEY, COMPACT_PARSER,

COMPACT_FILE,

quoted

+		     COMPACT_METADATA, COMPACT_DIGEST_LIST,

COMPACT__LAST };

quoted

+
+enum compact_modifiers { COMPACT_MOD_IMMUTABLE,

COMPACT_MOD__LAST };

quoted

+
+enum compact_actions { COMPACT_ACTION_IMA_MEASURED,
+		       COMPACT_ACTION_IMA_APPRAISED,
+		       COMPACT_ACTION_IMA_APPRAISED_DIGSIG,
+		       COMPACT_ACTION__LAST };
+
+enum ops { DIGEST_LIST_ADD, DIGEST_LIST_DEL, DIGEST_LIST_OP__LAST };
+

For enums you export to userspace, you need to specify the values so
that all compilers get them right.

quoted

+struct compact_list_hdr {
+	__u8 version;

You should never need a version, that way lies madness.

We wanted to have a way to switch to a new format, if necessary.

quoted

+	__u8 _reserved;

You better be testing this for 0, right?

Ok, will do.

quoted

+	__le16 type;
+	__le16 modifiers;
+	__le16 algo;
+	__le32 count;
+	__le32 datalen;

Why are user/kernel apis specified in little endian format?  Why would
that matter?  Shouldn't they just be "native" endian?

I thought this would make it clear that the kernel always expects the
digest lists to be in little endian.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

thanks,

greg k-h