On Sun, Aug 28, 2022 at 5:57 AM Jarkko Sakkinen [off-list ref] wrote:

On Fri, Aug 26, 2022 at 09:14:09AM +0200, Roberto Sassu wrote:

quoted

On Fri, 2022-08-26 at 08:42 +0300, Jarkko Sakkinen wrote:

quoted

On Thu, Aug 18, 2022 at 05:29:23PM +0200,
roberto.sassu@huaweicloud.com wrote:

quoted

From: Roberto Sassu <roberto.sassu@huawei.com>

In preparation for the patch that introduces the
bpf_lookup_user_key() eBPF
kfunc, move KEY_LOOKUP_ definitions to include/linux/key.h, to be
able to
validate the kfunc parameters.

Also, introduce key_lookup_flags_check() directly in
include/linux/key.h,
to reduce the risk that the check is not in sync with currently
defined
flags.

Missing the description what the heck this function even is.

Please, explain that.

Hi Jarkko

sorry, forgot to update the commit description. Will do it.

quoted

Also, the short subject is misleading because this *just*
does not move flags.

quoted

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/key.h      | 11 +++++++++++
 security/keys/internal.h |  2 --
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 7febc4881363..b5bbae77a9e7 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h

@@ -88,6 +88,17 @@ enum key_need_perm {
  KEY_DEFER_PERM_CHECK,   /* Special: permission check is

deferred */
 };

+#define KEY_LOOKUP_CREATE        0x01
+#define KEY_LOOKUP_PARTIAL       0x02
+

/*
 * Explain what the heck this function is.
 */

quoted

+static inline int key_lookup_flags_check(u64 flags)
+{
+ if (flags & ~(KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL))
+         return -EINVAL;
+
+ return 0;
+}

This is essentially a boolean function, right?

I.e. the implementation can be just:

!!(flags & ~(KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL))

Absolutely fine with that, if you prefer.

It can be either, it more depends on if a new function
is needed in the first place.

E.g. if you are worried about maintaining you could just
as well define a constant containing the mask, right?

+1 A mask is better.

quoted

quoted

Not even sure if this is needed in the first place, or
would it be better just to open code it. How many call
sites does it have anyway?

Daniel preferred to have this check here.

How many call sites?

BR, Jarkko