From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 48164CCF9F8 for ; Mon, 3 Nov 2025 11:03:50 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8463510E1A4; Mon, 3 Nov 2025 11:03:49 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="CQfx10Id"; dkim-atps=neutral Received: from mail-pl1-f178.google.com (mail-pl1-f178.google.com [209.85.214.178]) by gabe.freedesktop.org (Postfix) with ESMTPS id CBBC710E1A4 for ; Mon, 3 Nov 2025 11:03:47 +0000 (UTC) Received: by mail-pl1-f178.google.com with SMTP id d9443c01a7336-294f3105435so379535ad.1 for ; Mon, 03 Nov 2025 03:03:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1762167827; x=1762772627; darn=lists.freedesktop.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=JG+etGO4DCF1PIFwJUo34O2NHRXv7sGubVRA4h3tffk=; b=CQfx10IdZbSczTZDGaGrJzKY74WzA34tBAFmMzDnDfTY82fOYZ28bx2595bra3rCvC QZCgfLbYSJElnEHcJdq9xFG1ulYFBmrDUBLaOnTYWRPR861sWclU+9bGrOSEOZs7CS2w ia7Epjq03UaCifpxaKAkmed65wa3yxblaL4oU1x/ic3Ae5MFwO1G/qTDap1VUGG3BFBb kl9v08pqreXLndxn2AxpUJDiYVNhs+QxI5nPIi3NgDMGon1rxSHM9SEOCoSBgsAx9QMu Ewmiu4dAJXrR+CEZUZCOIjdBHi/4jW4saz4W+47n/IwNXuj2spC9yrEupHuNnoiAIzI2 sXhw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1762167827; x=1762772627; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=JG+etGO4DCF1PIFwJUo34O2NHRXv7sGubVRA4h3tffk=; b=BP/9jYuoEVJaMPI2FsAARAT2Mtfg4LmPJKIFxy/wTLIAUHocYXUwpFytptT6fGe+BL fTDuS3yqUHqzo1A5qZXxbRSHG1e/hCXucU8A2TWD8WpePtegk+AR4tIZ9rmpGy0CK/1O ojnR4KgT3OJJS6hCOwpkYWBm0Zldv+MHqg6GTZT+iVZ9J6T2TUbRAMDkEQvHq/p8dE7m 5Wj00K/6VwDXWFnzanudKau8/NAdf0jlH1oFWaiccZV0VrY23Kvq3kxDh4fSVygWXEZZ vKr57xlnGpC3fkzuAgveuatMllxQqMyx/ZVOWuihKYnHhJxGYWeE1itN4Zl0nMw4b9Hz zxNg== X-Forwarded-Encrypted: i=1; AJvYcCXSwEDNLWjFd41YNe0yjWWEFqGd8ejlBXi6uLZxjEzqCC6MRsQhFpU6aYWB9wxJr1Q42DLgrVoYK5U=@lists.freedesktop.org X-Gm-Message-State: AOJu0YwEZkPklRp0lPwTlGegwH0kWWKlHUW57ZX5bwewzkGBjLj9UFgf gN8Aw5M9hP2ITG6QOmHddN0svIizK5JnWbD2iSPCTW+0s1QvpvaPHLRVZdKSYT+1FQ== X-Gm-Gg: ASbGncvWvApDOkNoyHBZumwqkHQPDqzKzT4X3BRJa4o8rDaZIIWPsBvem1m1VX7hDU6 MzXXTGKEZKd0dBadQs7JBr35h240EndY4rpZcQdr8iMFnfiPm/pFSUEJXnDhd54C8A7ELoVfliy Nd1dg+W2SLMLZJp50RpCfF6ZiWgiRRBj1KsmMY6iRgZvLSAuVpPx/qtQcQCVlyfi1aoD1e4QAYT 6n3xkG4frBtieBZ/w9HHyO+4W8hvB+P77KzveeXMVoC5fZ/YNWpw3IBk0dZuXG/dWzhepI6TIlk MV6fJY75zFoEc2b/0YUAROEpacLYmfIWvHtcZbqcxq4KAD+Ue8bq/cMWIU6GvaSjS3psmI0SiSQ 1ZIRatCUr9GD2jSlt7pz+L5SapsgIOYTl0jbdNcBaz2wj3WNuTPit9u5f4ad1LbbrVoUnYiYKSg HYCckgU1vnM8brLXxiUIWoPKegqlHFICCEH63pGw== X-Google-Smtp-Source: AGHT+IGcDUbk6qqLdQjGL3Yxey3X8mTSWxpSXkjqaoDl6gxo10SeAyfD2HQnpWNy1nfGBwMDtJcUBw== X-Received: by 2002:a17:903:228c:b0:271:9873:80d9 with SMTP id d9443c01a7336-29556476bd0mr5742325ad.7.1762167826744; Mon, 03 Nov 2025 03:03:46 -0800 (PST) Received: from google.com (164.210.142.34.bc.googleusercontent.com. [34.142.210.164]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-29537cccdecsm102934645ad.19.2025.11.03.03.03.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Nov 2025 03:03:46 -0800 (PST) Date: Mon, 3 Nov 2025 11:03:36 +0000 From: Pranjal Shrivastava To: Jason Gunthorpe Cc: Alexander Gordeev , David Airlie , Alex Williamson , Ankit Agrawal , Christian Borntraeger , Brett Creeley , dri-devel@lists.freedesktop.org, Eric Auger , Eric Farman , Giovanni Cabiddu , Vasily Gorbik , Heiko Carstens , intel-gfx@lists.freedesktop.org, Jani Nikula , Joonas Lahtinen , Kevin Tian , kvm@vger.kernel.org, Kirti Wankhede , linux-s390@vger.kernel.org, Longfang Liu , Matthew Rosato , Nikhil Agarwal , Nipun Gupta , Peter Oberparleiter , Halil Pasic , qat-linux@intel.com, Rodrigo Vivi , Simona Vetter , Shameer Kolothum , Mostafa Saleh , Sven Schnelle , Tvrtko Ursulin , virtualization@lists.linux.dev, Vineeth Vijayan , Yishai Hadas , Zhenyu Wang , Zhi Wang , patches@lists.linux.dev Subject: Re: [PATCH 15/22] vfio: Add get_region_info_caps op Message-ID: References: <0-v1-679a6fa27d31+209-vfio_get_region_info_op_jgg@nvidia.com> <15-v1-679a6fa27d31+209-vfio_get_region_info_op_jgg@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Mon, Nov 03, 2025 at 10:16:56AM +0000, Pranjal Shrivastava wrote: > On Thu, Oct 23, 2025 at 08:09:29PM -0300, Jason Gunthorpe wrote: > > This op does the copy to/from user for the info and can return back > > a cap chain through a vfio_info_cap * result. > > > > Signed-off-by: Jason Gunthorpe > > --- > > drivers/vfio/vfio_main.c | 54 +++++++++++++++++++++++++++++++++++++--- > > include/linux/vfio.h | 4 +++ > > 2 files changed, 54 insertions(+), 4 deletions(-) > > The newly added vfio_get_region_info seems to pull-in common boilerplate > code (like copy_from_user, arg size validation) into the core code, > removing redundancy across all other vfio drivers. LGTM. I missed one thing in this patch (luckily caught it in patch 22): > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index f056e82ba35075..82e7d79b1f9fe2 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -1259,6 +1259,55 @@ static int vfio_ioctl_device_feature(struct vfio_device *device, > } > } > > +static long vfio_get_region_info(struct vfio_device *device, > + struct vfio_region_info __user *arg) > +{ > + unsigned long minsz = offsetofend(struct vfio_region_info, offset); > + struct vfio_region_info info = {}; > + int ret; > + > + if (copy_from_user(&info, arg, minsz)) > + return -EFAULT; > + if (info.argsz < minsz) > + return -EINVAL; > + > + if (device->ops->get_region_info_caps) { > + struct vfio_info_cap caps = { .buf = NULL, .size = 0 }; > + > + ret = device->ops->get_region_info_caps(device, &info, &caps); > + if (ret) > + return ret; device->ops->get_region_info_caps (via vfio_info_add_capability) can allocate caps.buf and then return an error for a different reason. The if (ret) check returns early and the kfree(caps.buf) on the success path is never reached. Should we add kfree(caps.buf) to the error path here? This keeps the allocation and cleanup logic centralized in the core code Let's either write comment saying that the get_region_info_caps op is required to free caps.buf before returning error OR add a kfree(caps.buf) here. > + > + if (caps.size) { > + info.flags |= VFIO_REGION_INFO_FLAG_CAPS; > + if (info.argsz < sizeof(info) + caps.size) { > + info.argsz = sizeof(info) + caps.size; > + info.cap_offset = 0; > + } else { > + vfio_info_cap_shift(&caps, sizeof(info)); > + if (copy_to_user(arg + 1, caps.buf, > + caps.size)) { > + kfree(caps.buf); > + return -EFAULT; > + } > + info.cap_offset = sizeof(info); > + } > + kfree(caps.buf); > + } > + > + if (copy_to_user(arg, &info, minsz)) > + return -EFAULT; > + } else if (device->ops->get_region_info) { > + ret = device->ops->get_region_info(device, arg); > + if (ret) > + return ret; With the above comment addressed, Reviewed-by: Pranjal Shrivastava Thanks, Praan