[PATCH] nvme-ioctl: move capable() admin check to the end

[PATCH] nvme-ioctl: move capable() admin check to the end

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This can be an expensive call on some distros. Move it to the end after
checking the cheaper ways to determine if the command is allowed.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 32c9bcf491a33..4d4453743d62c 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -18,15 +18,12 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 {
 	u32 effects;
 
-	if (capable(CAP_SYS_ADMIN))
-		return true;
-
 	/*
 	 * Do not allow unprivileged passthrough on partitions, as that allows an
 	 * escape from the containment of the partition.
 	 */
 	if (flags & NVME_IOCTL_PARTITION)
-		return false;
+		goto admin;
 
 	/*
 	 * Do not allow unprivileged processes to send vendor specific or fabrics
@@ -34,7 +31,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 */
 	if (c->common.opcode >= nvme_cmd_vendor_start ||
 	    c->common.opcode == nvme_fabrics_command)
-		return false;
+		goto admin;
 
 	/*
 	 * Do not allow unprivileged passthrough of admin commands except
@@ -53,7 +50,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 				return true;
 			}
 		}
-		return false;
+		goto admin;
 	}
 
 	/*
@@ -63,7 +60,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 */
 	effects = nvme_command_effects(ns->ctrl, ns, c->common.opcode);
 	if (!(effects & NVME_CMD_EFFECTS_CSUPP))
-		return false;
+		goto admin;
 
 	/*
 	 * Don't allow passthrough for command that have intrusive (or unknown)
@@ -72,16 +69,20 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC |
 			NVME_CMD_EFFECTS_UUID_SEL |
 			NVME_CMD_EFFECTS_SCOPE_MASK))
-		return false;
+		goto admin;
 
 	/*
 	 * Only allow I/O commands that transfer data to the controller or that
 	 * change the logical block contents if the file descriptor is open for
 	 * writing.
 	 */
-	if (nvme_is_write(c) || (effects & NVME_CMD_EFFECTS_LBCC))
-		return open_for_write;
+	if ((nvme_is_write(c) || (effects & NVME_CMD_EFFECTS_LBCC)) &&
+	    !open_for_write)
+		goto admin;
+
 	return true;
+admin:
+	return capable(CAP_SYS_ADMIN);
 }
 
 /*
-- 
2.34.1

Re: [PATCH] nvme-ioctl: move capable() admin check to the end

[Jens Axboe]

On 11/30/23 1:19 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This can be an expensive call on some distros. Move it to the end after
> checking the cheaper ways to determine if the command is allowed.

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH] nvme-ioctl: move capable() admin check to the end

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH] nvme-ioctl: move capable() admin check to the end

[Christoph Hellwig]

On Thu, Nov 30, 2023 at 12:19:19PM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This can be an expensive call on some distros.

Huh?  How would a distro matter for kernel code?

Re: [PATCH] nvme-ioctl: move capable() admin check to the end

[Keith Busch]

On Mon, Dec 04, 2023 at 09:30:34AM +0100, Christoph Hellwig wrote:
> On Thu, Nov 30, 2023 at 12:19:19PM -0800, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > This can be an expensive call on some distros.
> 
> Huh?  How would a distro matter for kernel code?

Security monitoring: audit, LSM, and bpf hooks all make this function
undesirable for a fast path check.

Re: [PATCH] nvme-ioctl: move capable() admin check to the end

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 07:33:01AM -0700, Keith Busch wrote:
> On Mon, Dec 04, 2023 at 09:30:34AM +0100, Christoph Hellwig wrote:
> > On Thu, Nov 30, 2023 at 12:19:19PM -0800, Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > This can be an expensive call on some distros.
> > 
> > Huh?  How would a distro matter for kernel code?
> 
> Security monitoring: audit, LSM, and bpf hooks all make this function
> undesirable for a fast path check.

So what matters is kernel configuration, not "distro".

Re: [PATCH] nvme-ioctl: move capable() admin check to the end

[Keith Busch]

On Mon, Dec 04, 2023 at 03:35:52PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 04, 2023 at 07:33:01AM -0700, Keith Busch wrote:
> > On Mon, Dec 04, 2023 at 09:30:34AM +0100, Christoph Hellwig wrote:
> > > On Thu, Nov 30, 2023 at 12:19:19PM -0800, Keith Busch wrote:
> > > > From: Keith Busch <kbusch@kernel.org>
> > > > 
> > > > This can be an expensive call on some distros.
> > > 
> > > Huh?  How would a distro matter for kernel code?
> > 
> > Security monitoring: audit, LSM, and bpf hooks all make this function
> > undesirable for a fast path check.
> 
> So what matters is kernel configuration, not "distro".

Yeah, fair enough: the overhead comes from the kernel config. So just
update the change log and good to go?

Re: [PATCH] nvme-ioctl: move capable() admin check to the end

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 08:42:55AM -0700, Keith Busch wrote:
> Yeah, fair enough: the overhead comes from the kernel config. So just
> update the change log and good to go?

Yes, with that:

Reviewed-by: Christoph Hellwig <hch@lst.de>