Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <004ec27a-cc3a-c75c-952b-ff371b82b8d1@schaufler-ca.com>
References: <004ec27a-cc3a-c75c-952b-ff371b82b8d1@schaufler-ca.com> <CAEjxPJ6pFdDfm55pv9bT3CV5DTFF9VqzRmG_Xi5bKNxPaGuOLg@mail.gmail.com> <158932282880.2885325.2688622278854566047.stgit@warthog.procyon.org.uk>
To:     Casey Schaufler <casey@schaufler-ca.com>
Cc:     dhowells@redhat.com, stephen.smalley.work@gmail.com,
        Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        Paul Moore <paul@paul-moore.com>, keyrings@vger.kernel.org,
        selinux@vger.kernel.org, linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3612471.1589412319.1@warthog.procyon.org.uk>
Date:   Thu, 14 May 2020 00:25:19 +0100
Message-ID: <3612472.1589412319@warthog.procyon.org.uk>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Casey Schaufler <casey@schaufler-ca.com> wrote:

> > -	if (perm & ~KEY_NEED_ALL)
> > -		return -EINVAL;
> > +	switch (need_perm) {
> > +	default:
> > +		return -EACCES;
> ...
> Is the change from -EINVAL to -EACCES a bug fix?
> Does it introduce an incompatibility?

It shouldn't happen.  All the actual cases should be covered explicitly in the
switch.  It's to catch a programming issue in the kernel where a new value
gets added to the enum but not propagated to all the places that check for it.

I'd actually prefer it to be something even more obvious, especially as EINVAL
is so widely used in the kernel.  Should I put a WARN_ON in there?

David