From: Jason Gunthorpe <jgg@nvidia.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: kbuild@lists.01.org, lkp@intel.com, kbuild-all@lists.01.org,
linux-kernel@vger.kernel.org
Subject: Re: [jgunthorpe:vfio_iommufd 9/11] drivers/vfio/vfio_main.c:1690 vfio_file_enforced_coherent() warn: assigning (-95) to unsigned variable 'ret'
Date: Tue, 25 Oct 2022 13:22:00 -0300 [thread overview]
Message-ID: <Y1gNKJ1QhdcAn3rn@nvidia.com> (raw)
In-Reply-To: <202210252103.10UKM7Mu-lkp@intel.com>
On Tue, Oct 25, 2022 at 05:21:36PM +0300, Dan Carpenter wrote:
> tree: https://github.com/jgunthorpe/linux vfio_iommufd
> head: a249441ba6fd9d658f4a1b568453e3a742d12686
> commit: e44299750e287f3d64d207a5af7abb021634a31b [9/11] vfio: Make vfio_container optionally compiled
> config: openrisc-randconfig-m041-20221024
> compiler: or1k-linux-gcc (GCC) 12.1.0
Dan! Thank you for looking at this branch, I'm going to be getting it
in linux-next very soon, so I will fix whatever your tools will spot!
Let me know
> New smatch warnings:
> drivers/vfio/vfio_main.c:1690 vfio_file_enforced_coherent() warn: assigning (-95) to unsigned variable 'ret'
>
> Old smatch warnings:
> drivers/vfio/vfio_main.c:1907 vfio_pin_pages() warn: impossible condition '(iova > (~0)) => (0-u32max > u32max)'
> drivers/vfio/vfio_main.c:1974 vfio_dma_rw() warn: impossible condition '(iova > (~0)) => (0-u32max > u32max)'
>
> vim +/ret +1690 drivers/vfio/vfio_main.c
>
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1671 /**
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1672 * vfio_file_enforced_coherent - True if the DMA associated with the VFIO file
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1673 * is always CPU cache coherent
>
> This comment sort of feels like returning false on error is the correct
> thing but in the rest of the code it seems like returning true on error
> is correct.
Oddly, true is the correct result. "false" means you have proven you
are worthy and that cannot happen on error cases.
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1674 * @file: VFIO group file
> c0560f51cf7747 drivers/vfio/vfio.c Yan Zhao 2020-03-24 1675 *
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1676 * Enforced coherency means that the IOMMU ignores things like the PCIe no-snoop
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1677 * bit in DMA transactions. A return of false indicates that the user has
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1678 * rights to access additional instructions such as wbinvd on x86.
> c0560f51cf7747 drivers/vfio/vfio.c Yan Zhao 2020-03-24 1679 */
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1680 bool vfio_file_enforced_coherent(struct file *file)
> c0560f51cf7747 drivers/vfio/vfio.c Yan Zhao 2020-03-24 1681 {
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1682 struct vfio_group *group = file->private_data;
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1683 bool ret;
> c0560f51cf7747 drivers/vfio/vfio.c Yan Zhao 2020-03-24 1684
> b1b8132a651cf6 drivers/vfio/vfio_main.c Alex Williamson 2022-10-07 1685 if (!vfio_file_is_group(file))
> a905ad043f32bb drivers/vfio/vfio.c Jason Gunthorpe 2022-05-04 1686 return true;
> c0560f51cf7747 drivers/vfio/vfio.c Yan Zhao 2020-03-24 1687
> c82e81ab256955 drivers/vfio/vfio_main.c Jason Gunthorpe 2022-09-29 1688 mutex_lock(&group->group_lock);
> e0e29bdb594adf drivers/vfio/vfio.c Jason Gunthorpe 2022-05-16 1689 if (group->container) {
> 1408640d578887 drivers/vfio/vfio_main.c Jason Gunthorpe 2022-09-22 @1690 ret = vfio_container_ioctl_check_extension(group->container,
> e0e29bdb594adf drivers/vfio/vfio.c Jason Gunthorpe 2022-05-16 1691 VFIO_DMA_CC_IOMMU);
>
> But this returns true if vfio_container_ioctl_check_extension() returns
> a negative error code. (I haven't looked at the git branch and I
> suspect it's different from linux-next)
Yes, I should not take this shortcut, we would be better to explicitly
return false on error return.
But good news, I just deleted this code, so all fixed :)
Jason
prev parent reply other threads:[~2022-10-25 16:22 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-25 14:21 [jgunthorpe:vfio_iommufd 9/11] drivers/vfio/vfio_main.c:1690 vfio_file_enforced_coherent() warn: assigning (-95) to unsigned variable 'ret' Dan Carpenter
2022-10-25 16:22 ` Jason Gunthorpe [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y1gNKJ1QhdcAn3rn@nvidia.com \
--to=jgg@nvidia.com \
--cc=dan.carpenter@oracle.com \
--cc=kbuild-all@lists.01.org \
--cc=kbuild@lists.01.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox