public inbox for [email protected]
 help / color / mirror / Atom feed
* [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
@ 2022-01-16 18:10 kernel test robot
  2022-01-19 20:38 ` Axel Rasmussen
  0 siblings, 1 reply; 9+ messages in thread
From: kernel test robot @ 2022-01-16 18:10 UTC (permalink / raw)
  To: Axel Rasmussen
  Cc: GNU/Weeb Mailing List, llvm, linux-kernel, Peter Xu,
	Linux Memory Management List, Lokesh Gidra, kbuild-all,
	Andrew Morton, Todd Kjos

Hi Axel,

FYI, the error/warning still remains.

tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
head:   bdf17ba628090156b539b1474eb5c636eeaf571b
commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
        git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
        git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
        git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
                   __u64 ioctls_out;
                         ^
   1 warning generated.


vim +/ioctls_out +1519 fs/userfaultfd.c

  1311	
  1312	static int userfaultfd_register(struct userfaultfd_ctx *ctx,
  1313					unsigned long arg)
  1314	{
  1315		struct mm_struct *mm = ctx->mm;
  1316		struct vm_area_struct *vma, *prev, *cur;
  1317		int ret;
  1318		struct uffdio_register uffdio_register;
  1319		struct uffdio_register __user *user_uffdio_register;
  1320		unsigned long vm_flags, new_flags;
  1321		bool found;
  1322		bool basic_ioctls;
  1323		unsigned long start, end, vma_end;
  1324	
  1325		user_uffdio_register = (struct uffdio_register __user *) arg;
  1326	
  1327		ret = -EFAULT;
  1328		if (copy_from_user(&uffdio_register, user_uffdio_register,
  1329				   sizeof(uffdio_register)-sizeof(__u64)))
  1330			goto out;
  1331	
  1332		ret = -EINVAL;
  1333		if (!uffdio_register.mode)
  1334			goto out;
  1335		if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
  1336			goto out;
  1337		vm_flags = 0;
  1338		if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
  1339			vm_flags |= VM_UFFD_MISSING;
  1340		if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
  1341			vm_flags |= VM_UFFD_WP;
  1342			/*
  1343			 * FIXME: remove the below error constraint by
  1344			 * implementing the wprotect tracking mode.
  1345			 */
  1346			ret = -EINVAL;
  1347			goto out;
  1348		}
  1349		if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
  1350	#ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
  1351			goto out;
  1352	#endif
  1353			vm_flags |= VM_UFFD_MINOR;
  1354		}
  1355	
  1356		ret = validate_range(mm, &uffdio_register.range.start,
  1357				     uffdio_register.range.len);
  1358		if (ret)
  1359			goto out;
  1360	
  1361		start = uffdio_register.range.start;
  1362		end = start + uffdio_register.range.len;
  1363	
  1364		ret = -ENOMEM;
  1365		if (!mmget_not_zero(mm))
  1366			goto out;
  1367	
  1368		down_write(&mm->mmap_sem);
  1369		if (!mmget_still_valid(mm))
  1370			goto out_unlock;
  1371		vma = find_vma_prev(mm, start, &prev);
  1372		if (!vma)
  1373			goto out_unlock;
  1374	
  1375		/* check that there's at least one vma in the range */
  1376		ret = -EINVAL;
  1377		if (vma->vm_start >= end)
  1378			goto out_unlock;
  1379	
  1380		/*
  1381		 * If the first vma contains huge pages, make sure start address
  1382		 * is aligned to huge page size.
  1383		 */
  1384		if (is_vm_hugetlb_page(vma)) {
  1385			unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
  1386	
  1387			if (start & (vma_hpagesize - 1))
  1388				goto out_unlock;
  1389		}
  1390	
  1391		/*
  1392		 * Search for not compatible vmas.
  1393		 */
  1394		found = false;
  1395		basic_ioctls = false;
  1396		for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
  1397			cond_resched();
  1398	
  1399			BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
  1400			       !!(cur->vm_flags & __VM_UFFD_FLAGS));
  1401	
  1402			/* check not compatible vmas */
  1403			ret = -EINVAL;
  1404			if (!vma_can_userfault(cur, vm_flags))
  1405				goto out_unlock;
  1406	
  1407			/*
  1408			 * UFFDIO_COPY will fill file holes even without
  1409			 * PROT_WRITE. This check enforces that if this is a
  1410			 * MAP_SHARED, the process has write permission to the backing
  1411			 * file. If VM_MAYWRITE is set it also enforces that on a
  1412			 * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
  1413			 * F_WRITE_SEAL can be taken until the vma is destroyed.
  1414			 */
  1415			ret = -EPERM;
  1416			if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
  1417				goto out_unlock;
  1418	
  1419			/*
  1420			 * If this vma contains ending address, and huge pages
  1421			 * check alignment.
  1422			 */
  1423			if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
  1424			    end > cur->vm_start) {
  1425				unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
  1426	
  1427				ret = -EINVAL;
  1428	
  1429				if (end & (vma_hpagesize - 1))
  1430					goto out_unlock;
  1431			}
  1432	
  1433			/*
  1434			 * Check that this vma isn't already owned by a
  1435			 * different userfaultfd. We can't allow more than one
  1436			 * userfaultfd to own a single vma simultaneously or we
  1437			 * wouldn't know which one to deliver the userfaults to.
  1438			 */
  1439			ret = -EBUSY;
  1440			if (cur->vm_userfaultfd_ctx.ctx &&
  1441			    cur->vm_userfaultfd_ctx.ctx != ctx)
  1442				goto out_unlock;
  1443	
  1444			/*
  1445			 * Note vmas containing huge pages
  1446			 */
  1447			if (is_vm_hugetlb_page(cur))
  1448				basic_ioctls = true;
  1449	
  1450			found = true;
  1451		}
  1452		BUG_ON(!found);
  1453	
  1454		if (vma->vm_start < start)
  1455			prev = vma;
  1456	
  1457		ret = 0;
  1458		do {
  1459			cond_resched();
  1460	
  1461			BUG_ON(!vma_can_userfault(vma, vm_flags));
  1462			BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
  1463			       vma->vm_userfaultfd_ctx.ctx != ctx);
  1464			WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
  1465	
  1466			/*
  1467			 * Nothing to do: this vma is already registered into this
  1468			 * userfaultfd and with the right tracking mode too.
  1469			 */
  1470			if (vma->vm_userfaultfd_ctx.ctx == ctx &&
  1471			    (vma->vm_flags & vm_flags) == vm_flags)
  1472				goto skip;
  1473	
  1474			if (vma->vm_start > start)
  1475				start = vma->vm_start;
  1476			vma_end = min(end, vma->vm_end);
  1477	
  1478			new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
  1479			prev = vma_merge(mm, prev, start, vma_end, new_flags,
  1480					 vma->anon_vma, vma->vm_file, vma->vm_pgoff,
  1481					 vma_policy(vma),
  1482					 ((struct vm_userfaultfd_ctx){ ctx }),
  1483					 vma_get_anon_name(vma));
  1484			if (prev) {
  1485				vma = prev;
  1486				goto next;
  1487			}
  1488			if (vma->vm_start < start) {
  1489				ret = split_vma(mm, vma, start, 1);
  1490				if (ret)
  1491					break;
  1492			}
  1493			if (vma->vm_end > end) {
  1494				ret = split_vma(mm, vma, end, 0);
  1495				if (ret)
  1496					break;
  1497			}
  1498		next:
  1499			/*
  1500			 * In the vma_merge() successful mprotect-like case 8:
  1501			 * the next vma was merged into the current one and
  1502			 * the current one has not been updated yet.
  1503			 */
  1504			vma->vm_flags = new_flags;
  1505			vma->vm_userfaultfd_ctx.ctx = ctx;
  1506	
  1507			if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
  1508				hugetlb_unshare_all_pmds(vma);
  1509	
  1510		skip:
  1511			prev = vma;
  1512			start = vma->vm_end;
  1513			vma = vma->vm_next;
  1514		} while (vma && vma->vm_start < end);
  1515	out_unlock:
  1516		up_write(&mm->mmap_sem);
  1517		mmput(mm);
  1518		if (!ret) {
> 1519			__u64 ioctls_out;
  1520	
  1521			ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
  1522			    UFFD_API_RANGE_IOCTLS;
  1523	
  1524			/* CONTINUE ioctl is only supported for MINOR ranges. */
  1525			if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
  1526				ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
  1527	
  1528			/*
  1529			 * Now that we scanned all vmas we can already tell
  1530			 * userland which ioctls methods are guaranteed to
  1531			 * succeed on this range.
  1532			 */
  1533			if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
  1534				     UFFD_API_RANGE_IOCTLS,
  1535				     &user_uffdio_register->ioctls))
  1536				ret = -EFAULT;
  1537		}
  1538	out:
  1539		return ret;
  1540	}
  1541	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-16 18:10 [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used kernel test robot
@ 2022-01-19 20:38 ` Axel Rasmussen
  2022-01-19 21:19   ` Suren Baghdasaryan
  0 siblings, 1 reply; 9+ messages in thread
From: Axel Rasmussen @ 2022-01-19 20:38 UTC (permalink / raw)
  To: kernel test robot
  Cc: GNU/Weeb Mailing List, llvm, LKML, Peter Xu,
	Linux Memory Management List, Lokesh Gidra, kbuild-all,
	Andrew Morton, Todd Kjos

Lokesh, I only spent a few moments looking, but my suspicion is that
this is the result of an incomplete backport of these patches to the
Android kernel?

The UFFDIO_CONTINUE patches introduced ioctls_out since the
computation of it got more complicated, and those same patches also
modified the put_user() on line 1533 here to use ioctls_out. I think
Android backported the first part, but left out the second?

On Sun, Jan 16, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
>
> Hi Axel,
>
> FYI, the error/warning still remains.
>
> tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
> head:   bdf17ba628090156b539b1474eb5c636eeaf571b
> commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
> config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
>         git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
>         git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
>         git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> >> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
>                    __u64 ioctls_out;
>                          ^
>    1 warning generated.
>
>
> vim +/ioctls_out +1519 fs/userfaultfd.c
>
>   1311
>   1312  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
>   1313                                  unsigned long arg)
>   1314  {
>   1315          struct mm_struct *mm = ctx->mm;
>   1316          struct vm_area_struct *vma, *prev, *cur;
>   1317          int ret;
>   1318          struct uffdio_register uffdio_register;
>   1319          struct uffdio_register __user *user_uffdio_register;
>   1320          unsigned long vm_flags, new_flags;
>   1321          bool found;
>   1322          bool basic_ioctls;
>   1323          unsigned long start, end, vma_end;
>   1324
>   1325          user_uffdio_register = (struct uffdio_register __user *) arg;
>   1326
>   1327          ret = -EFAULT;
>   1328          if (copy_from_user(&uffdio_register, user_uffdio_register,
>   1329                             sizeof(uffdio_register)-sizeof(__u64)))
>   1330                  goto out;
>   1331
>   1332          ret = -EINVAL;
>   1333          if (!uffdio_register.mode)
>   1334                  goto out;
>   1335          if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
>   1336                  goto out;
>   1337          vm_flags = 0;
>   1338          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
>   1339                  vm_flags |= VM_UFFD_MISSING;
>   1340          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
>   1341                  vm_flags |= VM_UFFD_WP;
>   1342                  /*
>   1343                   * FIXME: remove the below error constraint by
>   1344                   * implementing the wprotect tracking mode.
>   1345                   */
>   1346                  ret = -EINVAL;
>   1347                  goto out;
>   1348          }
>   1349          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
>   1350  #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
>   1351                  goto out;
>   1352  #endif
>   1353                  vm_flags |= VM_UFFD_MINOR;
>   1354          }
>   1355
>   1356          ret = validate_range(mm, &uffdio_register.range.start,
>   1357                               uffdio_register.range.len);
>   1358          if (ret)
>   1359                  goto out;
>   1360
>   1361          start = uffdio_register.range.start;
>   1362          end = start + uffdio_register.range.len;
>   1363
>   1364          ret = -ENOMEM;
>   1365          if (!mmget_not_zero(mm))
>   1366                  goto out;
>   1367
>   1368          down_write(&mm->mmap_sem);
>   1369          if (!mmget_still_valid(mm))
>   1370                  goto out_unlock;
>   1371          vma = find_vma_prev(mm, start, &prev);
>   1372          if (!vma)
>   1373                  goto out_unlock;
>   1374
>   1375          /* check that there's at least one vma in the range */
>   1376          ret = -EINVAL;
>   1377          if (vma->vm_start >= end)
>   1378                  goto out_unlock;
>   1379
>   1380          /*
>   1381           * If the first vma contains huge pages, make sure start address
>   1382           * is aligned to huge page size.
>   1383           */
>   1384          if (is_vm_hugetlb_page(vma)) {
>   1385                  unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
>   1386
>   1387                  if (start & (vma_hpagesize - 1))
>   1388                          goto out_unlock;
>   1389          }
>   1390
>   1391          /*
>   1392           * Search for not compatible vmas.
>   1393           */
>   1394          found = false;
>   1395          basic_ioctls = false;
>   1396          for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
>   1397                  cond_resched();
>   1398
>   1399                  BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
>   1400                         !!(cur->vm_flags & __VM_UFFD_FLAGS));
>   1401
>   1402                  /* check not compatible vmas */
>   1403                  ret = -EINVAL;
>   1404                  if (!vma_can_userfault(cur, vm_flags))
>   1405                          goto out_unlock;
>   1406
>   1407                  /*
>   1408                   * UFFDIO_COPY will fill file holes even without
>   1409                   * PROT_WRITE. This check enforces that if this is a
>   1410                   * MAP_SHARED, the process has write permission to the backing
>   1411                   * file. If VM_MAYWRITE is set it also enforces that on a
>   1412                   * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
>   1413                   * F_WRITE_SEAL can be taken until the vma is destroyed.
>   1414                   */
>   1415                  ret = -EPERM;
>   1416                  if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
>   1417                          goto out_unlock;
>   1418
>   1419                  /*
>   1420                   * If this vma contains ending address, and huge pages
>   1421                   * check alignment.
>   1422                   */
>   1423                  if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
>   1424                      end > cur->vm_start) {
>   1425                          unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
>   1426
>   1427                          ret = -EINVAL;
>   1428
>   1429                          if (end & (vma_hpagesize - 1))
>   1430                                  goto out_unlock;
>   1431                  }
>   1432
>   1433                  /*
>   1434                   * Check that this vma isn't already owned by a
>   1435                   * different userfaultfd. We can't allow more than one
>   1436                   * userfaultfd to own a single vma simultaneously or we
>   1437                   * wouldn't know which one to deliver the userfaults to.
>   1438                   */
>   1439                  ret = -EBUSY;
>   1440                  if (cur->vm_userfaultfd_ctx.ctx &&
>   1441                      cur->vm_userfaultfd_ctx.ctx != ctx)
>   1442                          goto out_unlock;
>   1443
>   1444                  /*
>   1445                   * Note vmas containing huge pages
>   1446                   */
>   1447                  if (is_vm_hugetlb_page(cur))
>   1448                          basic_ioctls = true;
>   1449
>   1450                  found = true;
>   1451          }
>   1452          BUG_ON(!found);
>   1453
>   1454          if (vma->vm_start < start)
>   1455                  prev = vma;
>   1456
>   1457          ret = 0;
>   1458          do {
>   1459                  cond_resched();
>   1460
>   1461                  BUG_ON(!vma_can_userfault(vma, vm_flags));
>   1462                  BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
>   1463                         vma->vm_userfaultfd_ctx.ctx != ctx);
>   1464                  WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
>   1465
>   1466                  /*
>   1467                   * Nothing to do: this vma is already registered into this
>   1468                   * userfaultfd and with the right tracking mode too.
>   1469                   */
>   1470                  if (vma->vm_userfaultfd_ctx.ctx == ctx &&
>   1471                      (vma->vm_flags & vm_flags) == vm_flags)
>   1472                          goto skip;
>   1473
>   1474                  if (vma->vm_start > start)
>   1475                          start = vma->vm_start;
>   1476                  vma_end = min(end, vma->vm_end);
>   1477
>   1478                  new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
>   1479                  prev = vma_merge(mm, prev, start, vma_end, new_flags,
>   1480                                   vma->anon_vma, vma->vm_file, vma->vm_pgoff,
>   1481                                   vma_policy(vma),
>   1482                                   ((struct vm_userfaultfd_ctx){ ctx }),
>   1483                                   vma_get_anon_name(vma));
>   1484                  if (prev) {
>   1485                          vma = prev;
>   1486                          goto next;
>   1487                  }
>   1488                  if (vma->vm_start < start) {
>   1489                          ret = split_vma(mm, vma, start, 1);
>   1490                          if (ret)
>   1491                                  break;
>   1492                  }
>   1493                  if (vma->vm_end > end) {
>   1494                          ret = split_vma(mm, vma, end, 0);
>   1495                          if (ret)
>   1496                                  break;
>   1497                  }
>   1498          next:
>   1499                  /*
>   1500                   * In the vma_merge() successful mprotect-like case 8:
>   1501                   * the next vma was merged into the current one and
>   1502                   * the current one has not been updated yet.
>   1503                   */
>   1504                  vma->vm_flags = new_flags;
>   1505                  vma->vm_userfaultfd_ctx.ctx = ctx;
>   1506
>   1507                  if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
>   1508                          hugetlb_unshare_all_pmds(vma);
>   1509
>   1510          skip:
>   1511                  prev = vma;
>   1512                  start = vma->vm_end;
>   1513                  vma = vma->vm_next;
>   1514          } while (vma && vma->vm_start < end);
>   1515  out_unlock:
>   1516          up_write(&mm->mmap_sem);
>   1517          mmput(mm);
>   1518          if (!ret) {
> > 1519                  __u64 ioctls_out;
>   1520
>   1521                  ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
>   1522                      UFFD_API_RANGE_IOCTLS;
>   1523
>   1524                  /* CONTINUE ioctl is only supported for MINOR ranges. */
>   1525                  if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
>   1526                          ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
>   1527
>   1528                  /*
>   1529                   * Now that we scanned all vmas we can already tell
>   1530                   * userland which ioctls methods are guaranteed to
>   1531                   * succeed on this range.
>   1532                   */
>   1533                  if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
>   1534                               UFFD_API_RANGE_IOCTLS,
>   1535                               &user_uffdio_register->ioctls))
>   1536                          ret = -EFAULT;
>   1537          }
>   1538  out:
>   1539          return ret;
>   1540  }
>   1541
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-19 20:38 ` Axel Rasmussen
@ 2022-01-19 21:19   ` Suren Baghdasaryan
  2022-01-19 22:02     ` Axel Rasmussen
  0 siblings, 1 reply; 9+ messages in thread
From: Suren Baghdasaryan @ 2022-01-19 21:19 UTC (permalink / raw)
  To: Axel Rasmussen
  Cc: GNU/Weeb Mailing List, kernel test robot, llvm, LKML, Peter Xu,
	Linux Memory Management List, Lokesh Gidra, Kalesh Singh,
	kbuild-all, Andrew Morton, Todd Kjos

On Wed, Jan 19, 2022 at 12:39 PM Axel Rasmussen
<[email protected]> wrote:
>
> Lokesh, I only spent a few moments looking, but my suspicion is that
> this is the result of an incomplete backport of these patches to the
> Android kernel?
>
> The UFFDIO_CONTINUE patches introduced ioctls_out since the
> computation of it got more complicated, and those same patches also
> modified the put_user() on line 1533 here to use ioctls_out. I think
> Android backported the first part, but left out the second?

The backport https://android-review.googlesource.com/c/kernel/common/+/1652718
looks like a mixture of
14819305e09fe4 ("userfaultfd: wp: declare _UFFDIO_WRITEPROTECT
conditionally") and f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE
ioctl").
Not sure why it was done this way, maybe to minimize code changes
while backporting.
Simplest fix is probably to add __maybe_unused to ioctls_out declaration.

>
> On Sun, Jan 16, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
> >
> > Hi Axel,
> >
> > FYI, the error/warning still remains.
> >
> > tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
> > head:   bdf17ba628090156b539b1474eb5c636eeaf571b
> > commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
> > config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
> > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> >         git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
> >         git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
> >         git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> >         # save the config file to linux build tree
> >         mkdir build_dir
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
> >                    __u64 ioctls_out;
> >                          ^
> >    1 warning generated.
> >
> >
> > vim +/ioctls_out +1519 fs/userfaultfd.c
> >
> >   1311
> >   1312  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> >   1313                                  unsigned long arg)
> >   1314  {
> >   1315          struct mm_struct *mm = ctx->mm;
> >   1316          struct vm_area_struct *vma, *prev, *cur;
> >   1317          int ret;
> >   1318          struct uffdio_register uffdio_register;
> >   1319          struct uffdio_register __user *user_uffdio_register;
> >   1320          unsigned long vm_flags, new_flags;
> >   1321          bool found;
> >   1322          bool basic_ioctls;
> >   1323          unsigned long start, end, vma_end;
> >   1324
> >   1325          user_uffdio_register = (struct uffdio_register __user *) arg;
> >   1326
> >   1327          ret = -EFAULT;
> >   1328          if (copy_from_user(&uffdio_register, user_uffdio_register,
> >   1329                             sizeof(uffdio_register)-sizeof(__u64)))
> >   1330                  goto out;
> >   1331
> >   1332          ret = -EINVAL;
> >   1333          if (!uffdio_register.mode)
> >   1334                  goto out;
> >   1335          if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
> >   1336                  goto out;
> >   1337          vm_flags = 0;
> >   1338          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
> >   1339                  vm_flags |= VM_UFFD_MISSING;
> >   1340          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
> >   1341                  vm_flags |= VM_UFFD_WP;
> >   1342                  /*
> >   1343                   * FIXME: remove the below error constraint by
> >   1344                   * implementing the wprotect tracking mode.
> >   1345                   */
> >   1346                  ret = -EINVAL;
> >   1347                  goto out;
> >   1348          }
> >   1349          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
> >   1350  #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> >   1351                  goto out;
> >   1352  #endif
> >   1353                  vm_flags |= VM_UFFD_MINOR;
> >   1354          }
> >   1355
> >   1356          ret = validate_range(mm, &uffdio_register.range.start,
> >   1357                               uffdio_register.range.len);
> >   1358          if (ret)
> >   1359                  goto out;
> >   1360
> >   1361          start = uffdio_register.range.start;
> >   1362          end = start + uffdio_register.range.len;
> >   1363
> >   1364          ret = -ENOMEM;
> >   1365          if (!mmget_not_zero(mm))
> >   1366                  goto out;
> >   1367
> >   1368          down_write(&mm->mmap_sem);
> >   1369          if (!mmget_still_valid(mm))
> >   1370                  goto out_unlock;
> >   1371          vma = find_vma_prev(mm, start, &prev);
> >   1372          if (!vma)
> >   1373                  goto out_unlock;
> >   1374
> >   1375          /* check that there's at least one vma in the range */
> >   1376          ret = -EINVAL;
> >   1377          if (vma->vm_start >= end)
> >   1378                  goto out_unlock;
> >   1379
> >   1380          /*
> >   1381           * If the first vma contains huge pages, make sure start address
> >   1382           * is aligned to huge page size.
> >   1383           */
> >   1384          if (is_vm_hugetlb_page(vma)) {
> >   1385                  unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
> >   1386
> >   1387                  if (start & (vma_hpagesize - 1))
> >   1388                          goto out_unlock;
> >   1389          }
> >   1390
> >   1391          /*
> >   1392           * Search for not compatible vmas.
> >   1393           */
> >   1394          found = false;
> >   1395          basic_ioctls = false;
> >   1396          for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
> >   1397                  cond_resched();
> >   1398
> >   1399                  BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
> >   1400                         !!(cur->vm_flags & __VM_UFFD_FLAGS));
> >   1401
> >   1402                  /* check not compatible vmas */
> >   1403                  ret = -EINVAL;
> >   1404                  if (!vma_can_userfault(cur, vm_flags))
> >   1405                          goto out_unlock;
> >   1406
> >   1407                  /*
> >   1408                   * UFFDIO_COPY will fill file holes even without
> >   1409                   * PROT_WRITE. This check enforces that if this is a
> >   1410                   * MAP_SHARED, the process has write permission to the backing
> >   1411                   * file. If VM_MAYWRITE is set it also enforces that on a
> >   1412                   * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
> >   1413                   * F_WRITE_SEAL can be taken until the vma is destroyed.
> >   1414                   */
> >   1415                  ret = -EPERM;
> >   1416                  if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
> >   1417                          goto out_unlock;
> >   1418
> >   1419                  /*
> >   1420                   * If this vma contains ending address, and huge pages
> >   1421                   * check alignment.
> >   1422                   */
> >   1423                  if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
> >   1424                      end > cur->vm_start) {
> >   1425                          unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
> >   1426
> >   1427                          ret = -EINVAL;
> >   1428
> >   1429                          if (end & (vma_hpagesize - 1))
> >   1430                                  goto out_unlock;
> >   1431                  }
> >   1432
> >   1433                  /*
> >   1434                   * Check that this vma isn't already owned by a
> >   1435                   * different userfaultfd. We can't allow more than one
> >   1436                   * userfaultfd to own a single vma simultaneously or we
> >   1437                   * wouldn't know which one to deliver the userfaults to.
> >   1438                   */
> >   1439                  ret = -EBUSY;
> >   1440                  if (cur->vm_userfaultfd_ctx.ctx &&
> >   1441                      cur->vm_userfaultfd_ctx.ctx != ctx)
> >   1442                          goto out_unlock;
> >   1443
> >   1444                  /*
> >   1445                   * Note vmas containing huge pages
> >   1446                   */
> >   1447                  if (is_vm_hugetlb_page(cur))
> >   1448                          basic_ioctls = true;
> >   1449
> >   1450                  found = true;
> >   1451          }
> >   1452          BUG_ON(!found);
> >   1453
> >   1454          if (vma->vm_start < start)
> >   1455                  prev = vma;
> >   1456
> >   1457          ret = 0;
> >   1458          do {
> >   1459                  cond_resched();
> >   1460
> >   1461                  BUG_ON(!vma_can_userfault(vma, vm_flags));
> >   1462                  BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> >   1463                         vma->vm_userfaultfd_ctx.ctx != ctx);
> >   1464                  WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> >   1465
> >   1466                  /*
> >   1467                   * Nothing to do: this vma is already registered into this
> >   1468                   * userfaultfd and with the right tracking mode too.
> >   1469                   */
> >   1470                  if (vma->vm_userfaultfd_ctx.ctx == ctx &&
> >   1471                      (vma->vm_flags & vm_flags) == vm_flags)
> >   1472                          goto skip;
> >   1473
> >   1474                  if (vma->vm_start > start)
> >   1475                          start = vma->vm_start;
> >   1476                  vma_end = min(end, vma->vm_end);
> >   1477
> >   1478                  new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
> >   1479                  prev = vma_merge(mm, prev, start, vma_end, new_flags,
> >   1480                                   vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> >   1481                                   vma_policy(vma),
> >   1482                                   ((struct vm_userfaultfd_ctx){ ctx }),
> >   1483                                   vma_get_anon_name(vma));
> >   1484                  if (prev) {
> >   1485                          vma = prev;
> >   1486                          goto next;
> >   1487                  }
> >   1488                  if (vma->vm_start < start) {
> >   1489                          ret = split_vma(mm, vma, start, 1);
> >   1490                          if (ret)
> >   1491                                  break;
> >   1492                  }
> >   1493                  if (vma->vm_end > end) {
> >   1494                          ret = split_vma(mm, vma, end, 0);
> >   1495                          if (ret)
> >   1496                                  break;
> >   1497                  }
> >   1498          next:
> >   1499                  /*
> >   1500                   * In the vma_merge() successful mprotect-like case 8:
> >   1501                   * the next vma was merged into the current one and
> >   1502                   * the current one has not been updated yet.
> >   1503                   */
> >   1504                  vma->vm_flags = new_flags;
> >   1505                  vma->vm_userfaultfd_ctx.ctx = ctx;
> >   1506
> >   1507                  if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
> >   1508                          hugetlb_unshare_all_pmds(vma);
> >   1509
> >   1510          skip:
> >   1511                  prev = vma;
> >   1512                  start = vma->vm_end;
> >   1513                  vma = vma->vm_next;
> >   1514          } while (vma && vma->vm_start < end);
> >   1515  out_unlock:
> >   1516          up_write(&mm->mmap_sem);
> >   1517          mmput(mm);
> >   1518          if (!ret) {
> > > 1519                  __u64 ioctls_out;
> >   1520
> >   1521                  ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> >   1522                      UFFD_API_RANGE_IOCTLS;
> >   1523
> >   1524                  /* CONTINUE ioctl is only supported for MINOR ranges. */
> >   1525                  if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
> >   1526                          ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
> >   1527
> >   1528                  /*
> >   1529                   * Now that we scanned all vmas we can already tell
> >   1530                   * userland which ioctls methods are guaranteed to
> >   1531                   * succeed on this range.
> >   1532                   */
> >   1533                  if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> >   1534                               UFFD_API_RANGE_IOCTLS,
> >   1535                               &user_uffdio_register->ioctls))
> >   1536                          ret = -EFAULT;
> >   1537          }
> >   1538  out:
> >   1539          return ret;
> >   1540  }
> >   1541
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/[email protected]
>
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-19 21:19   ` Suren Baghdasaryan
@ 2022-01-19 22:02     ` Axel Rasmussen
  2022-01-19 22:10       ` Kalesh Singh
  2022-01-19 22:11       ` Lokesh Gidra
  0 siblings, 2 replies; 9+ messages in thread
From: Axel Rasmussen @ 2022-01-19 22:02 UTC (permalink / raw)
  To: Suren Baghdasaryan
  Cc: GNU/Weeb Mailing List, kernel test robot, llvm, LKML, Peter Xu,
	Linux Memory Management List, Lokesh Gidra, Kalesh Singh,
	kbuild-all, Andrew Morton, Todd Kjos

On Wed, Jan 19, 2022 at 1:19 PM Suren Baghdasaryan <[email protected]> wrote:
>
> On Wed, Jan 19, 2022 at 12:39 PM Axel Rasmussen
> <[email protected]> wrote:
> >
> > Lokesh, I only spent a few moments looking, but my suspicion is that
> > this is the result of an incomplete backport of these patches to the
> > Android kernel?
> >
> > The UFFDIO_CONTINUE patches introduced ioctls_out since the
> > computation of it got more complicated, and those same patches also
> > modified the put_user() on line 1533 here to use ioctls_out. I think
> > Android backported the first part, but left out the second?
>
> The backport https://android-review.googlesource.com/c/kernel/common/+/1652718
> looks like a mixture of
> 14819305e09fe4 ("userfaultfd: wp: declare _UFFDIO_WRITEPROTECT
> conditionally") and f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE
> ioctl").
> Not sure why it was done this way, maybe to minimize code changes
> while backporting.
> Simplest fix is probably to add __maybe_unused to ioctls_out declaration.

Unfortunately, that doesn't quite do it. That would fix the warning,
but the code in the original e-mail is still wrong: we incorrectly
don't report the _UFFDIO_CONTINUE bit, unless we use the value we
computed in ioctls_out.

>
> >
> > On Sun, Jan 16, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
> > >
> > > Hi Axel,
> > >
> > > FYI, the error/warning still remains.
> > >
> > > tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
> > > head:   bdf17ba628090156b539b1474eb5c636eeaf571b
> > > commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
> > > config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
> > > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
> > > reproduce (this is a W=1 build):
> > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >         chmod +x ~/bin/make.cross
> > >         # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > >         git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
> > >         git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
> > >         git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > >         # save the config file to linux build tree
> > >         mkdir build_dir
> > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> > >
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <[email protected]>
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > > >> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
> > >                    __u64 ioctls_out;
> > >                          ^
> > >    1 warning generated.
> > >
> > >
> > > vim +/ioctls_out +1519 fs/userfaultfd.c
> > >
> > >   1311
> > >   1312  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > >   1313                                  unsigned long arg)
> > >   1314  {
> > >   1315          struct mm_struct *mm = ctx->mm;
> > >   1316          struct vm_area_struct *vma, *prev, *cur;
> > >   1317          int ret;
> > >   1318          struct uffdio_register uffdio_register;
> > >   1319          struct uffdio_register __user *user_uffdio_register;
> > >   1320          unsigned long vm_flags, new_flags;
> > >   1321          bool found;
> > >   1322          bool basic_ioctls;
> > >   1323          unsigned long start, end, vma_end;
> > >   1324
> > >   1325          user_uffdio_register = (struct uffdio_register __user *) arg;
> > >   1326
> > >   1327          ret = -EFAULT;
> > >   1328          if (copy_from_user(&uffdio_register, user_uffdio_register,
> > >   1329                             sizeof(uffdio_register)-sizeof(__u64)))
> > >   1330                  goto out;
> > >   1331
> > >   1332          ret = -EINVAL;
> > >   1333          if (!uffdio_register.mode)
> > >   1334                  goto out;
> > >   1335          if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
> > >   1336                  goto out;
> > >   1337          vm_flags = 0;
> > >   1338          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
> > >   1339                  vm_flags |= VM_UFFD_MISSING;
> > >   1340          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
> > >   1341                  vm_flags |= VM_UFFD_WP;
> > >   1342                  /*
> > >   1343                   * FIXME: remove the below error constraint by
> > >   1344                   * implementing the wprotect tracking mode.
> > >   1345                   */
> > >   1346                  ret = -EINVAL;
> > >   1347                  goto out;
> > >   1348          }
> > >   1349          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
> > >   1350  #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> > >   1351                  goto out;
> > >   1352  #endif
> > >   1353                  vm_flags |= VM_UFFD_MINOR;
> > >   1354          }
> > >   1355
> > >   1356          ret = validate_range(mm, &uffdio_register.range.start,
> > >   1357                               uffdio_register.range.len);
> > >   1358          if (ret)
> > >   1359                  goto out;
> > >   1360
> > >   1361          start = uffdio_register.range.start;
> > >   1362          end = start + uffdio_register.range.len;
> > >   1363
> > >   1364          ret = -ENOMEM;
> > >   1365          if (!mmget_not_zero(mm))
> > >   1366                  goto out;
> > >   1367
> > >   1368          down_write(&mm->mmap_sem);
> > >   1369          if (!mmget_still_valid(mm))
> > >   1370                  goto out_unlock;
> > >   1371          vma = find_vma_prev(mm, start, &prev);
> > >   1372          if (!vma)
> > >   1373                  goto out_unlock;
> > >   1374
> > >   1375          /* check that there's at least one vma in the range */
> > >   1376          ret = -EINVAL;
> > >   1377          if (vma->vm_start >= end)
> > >   1378                  goto out_unlock;
> > >   1379
> > >   1380          /*
> > >   1381           * If the first vma contains huge pages, make sure start address
> > >   1382           * is aligned to huge page size.
> > >   1383           */
> > >   1384          if (is_vm_hugetlb_page(vma)) {
> > >   1385                  unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
> > >   1386
> > >   1387                  if (start & (vma_hpagesize - 1))
> > >   1388                          goto out_unlock;
> > >   1389          }
> > >   1390
> > >   1391          /*
> > >   1392           * Search for not compatible vmas.
> > >   1393           */
> > >   1394          found = false;
> > >   1395          basic_ioctls = false;
> > >   1396          for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
> > >   1397                  cond_resched();
> > >   1398
> > >   1399                  BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
> > >   1400                         !!(cur->vm_flags & __VM_UFFD_FLAGS));
> > >   1401
> > >   1402                  /* check not compatible vmas */
> > >   1403                  ret = -EINVAL;
> > >   1404                  if (!vma_can_userfault(cur, vm_flags))
> > >   1405                          goto out_unlock;
> > >   1406
> > >   1407                  /*
> > >   1408                   * UFFDIO_COPY will fill file holes even without
> > >   1409                   * PROT_WRITE. This check enforces that if this is a
> > >   1410                   * MAP_SHARED, the process has write permission to the backing
> > >   1411                   * file. If VM_MAYWRITE is set it also enforces that on a
> > >   1412                   * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
> > >   1413                   * F_WRITE_SEAL can be taken until the vma is destroyed.
> > >   1414                   */
> > >   1415                  ret = -EPERM;
> > >   1416                  if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
> > >   1417                          goto out_unlock;
> > >   1418
> > >   1419                  /*
> > >   1420                   * If this vma contains ending address, and huge pages
> > >   1421                   * check alignment.
> > >   1422                   */
> > >   1423                  if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
> > >   1424                      end > cur->vm_start) {
> > >   1425                          unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
> > >   1426
> > >   1427                          ret = -EINVAL;
> > >   1428
> > >   1429                          if (end & (vma_hpagesize - 1))
> > >   1430                                  goto out_unlock;
> > >   1431                  }
> > >   1432
> > >   1433                  /*
> > >   1434                   * Check that this vma isn't already owned by a
> > >   1435                   * different userfaultfd. We can't allow more than one
> > >   1436                   * userfaultfd to own a single vma simultaneously or we
> > >   1437                   * wouldn't know which one to deliver the userfaults to.
> > >   1438                   */
> > >   1439                  ret = -EBUSY;
> > >   1440                  if (cur->vm_userfaultfd_ctx.ctx &&
> > >   1441                      cur->vm_userfaultfd_ctx.ctx != ctx)
> > >   1442                          goto out_unlock;
> > >   1443
> > >   1444                  /*
> > >   1445                   * Note vmas containing huge pages
> > >   1446                   */
> > >   1447                  if (is_vm_hugetlb_page(cur))
> > >   1448                          basic_ioctls = true;
> > >   1449
> > >   1450                  found = true;
> > >   1451          }
> > >   1452          BUG_ON(!found);
> > >   1453
> > >   1454          if (vma->vm_start < start)
> > >   1455                  prev = vma;
> > >   1456
> > >   1457          ret = 0;
> > >   1458          do {
> > >   1459                  cond_resched();
> > >   1460
> > >   1461                  BUG_ON(!vma_can_userfault(vma, vm_flags));
> > >   1462                  BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> > >   1463                         vma->vm_userfaultfd_ctx.ctx != ctx);
> > >   1464                  WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> > >   1465
> > >   1466                  /*
> > >   1467                   * Nothing to do: this vma is already registered into this
> > >   1468                   * userfaultfd and with the right tracking mode too.
> > >   1469                   */
> > >   1470                  if (vma->vm_userfaultfd_ctx.ctx == ctx &&
> > >   1471                      (vma->vm_flags & vm_flags) == vm_flags)
> > >   1472                          goto skip;
> > >   1473
> > >   1474                  if (vma->vm_start > start)
> > >   1475                          start = vma->vm_start;
> > >   1476                  vma_end = min(end, vma->vm_end);
> > >   1477
> > >   1478                  new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
> > >   1479                  prev = vma_merge(mm, prev, start, vma_end, new_flags,
> > >   1480                                   vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> > >   1481                                   vma_policy(vma),
> > >   1482                                   ((struct vm_userfaultfd_ctx){ ctx }),
> > >   1483                                   vma_get_anon_name(vma));
> > >   1484                  if (prev) {
> > >   1485                          vma = prev;
> > >   1486                          goto next;
> > >   1487                  }
> > >   1488                  if (vma->vm_start < start) {
> > >   1489                          ret = split_vma(mm, vma, start, 1);
> > >   1490                          if (ret)
> > >   1491                                  break;
> > >   1492                  }
> > >   1493                  if (vma->vm_end > end) {
> > >   1494                          ret = split_vma(mm, vma, end, 0);
> > >   1495                          if (ret)
> > >   1496                                  break;
> > >   1497                  }
> > >   1498          next:
> > >   1499                  /*
> > >   1500                   * In the vma_merge() successful mprotect-like case 8:
> > >   1501                   * the next vma was merged into the current one and
> > >   1502                   * the current one has not been updated yet.
> > >   1503                   */
> > >   1504                  vma->vm_flags = new_flags;
> > >   1505                  vma->vm_userfaultfd_ctx.ctx = ctx;
> > >   1506
> > >   1507                  if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
> > >   1508                          hugetlb_unshare_all_pmds(vma);
> > >   1509
> > >   1510          skip:
> > >   1511                  prev = vma;
> > >   1512                  start = vma->vm_end;
> > >   1513                  vma = vma->vm_next;
> > >   1514          } while (vma && vma->vm_start < end);
> > >   1515  out_unlock:
> > >   1516          up_write(&mm->mmap_sem);
> > >   1517          mmput(mm);
> > >   1518          if (!ret) {
> > > > 1519                  __u64 ioctls_out;
> > >   1520
> > >   1521                  ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > >   1522                      UFFD_API_RANGE_IOCTLS;
> > >   1523
> > >   1524                  /* CONTINUE ioctl is only supported for MINOR ranges. */
> > >   1525                  if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
> > >   1526                          ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
> > >   1527
> > >   1528                  /*
> > >   1529                   * Now that we scanned all vmas we can already tell
> > >   1530                   * userland which ioctls methods are guaranteed to
> > >   1531                   * succeed on this range.
> > >   1532                   */
> > >   1533                  if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > >   1534                               UFFD_API_RANGE_IOCTLS,
> > >   1535                               &user_uffdio_register->ioctls))
> > >   1536                          ret = -EFAULT;
> > >   1537          }
> > >   1538  out:
> > >   1539          return ret;
> > >   1540  }
> > >   1541
> > >
> > > ---
> > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > https://lists.01.org/hyperkitty/list/[email protected]
> >
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-19 22:02     ` Axel Rasmussen
@ 2022-01-19 22:10       ` Kalesh Singh
  2022-01-19 22:11       ` Lokesh Gidra
  1 sibling, 0 replies; 9+ messages in thread
From: Kalesh Singh @ 2022-01-19 22:10 UTC (permalink / raw)
  To: Axel Rasmussen
  Cc: kbuild-all, kernel test robot, llvm, LKML, Peter Xu,
	Linux Memory Management List, Lokesh Gidra, GNU/Weeb Mailing List,
	Andrew Morton, Suren Baghdasaryan, Todd Kjos

On Wed, Jan 19, 2022 at 2:03 PM Axel Rasmussen <[email protected]> wrote:
>
> On Wed, Jan 19, 2022 at 1:19 PM Suren Baghdasaryan <[email protected]> wrote:
> >
> > On Wed, Jan 19, 2022 at 12:39 PM Axel Rasmussen
> > <[email protected]> wrote:
> > >
> > > Lokesh, I only spent a few moments looking, but my suspicion is that
> > > this is the result of an incomplete backport of these patches to the
> > > Android kernel?
> > >
> > > The UFFDIO_CONTINUE patches introduced ioctls_out since the
> > > computation of it got more complicated, and those same patches also
> > > modified the put_user() on line 1533 here to use ioctls_out. I think
> > > Android backported the first part, but left out the second?
> >
> > The backport https://android-review.googlesource.com/c/kernel/common/+/1652718
> > looks like a mixture of
> > 14819305e09fe4 ("userfaultfd: wp: declare _UFFDIO_WRITEPROTECT
> > conditionally") and f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE
> > ioctl").
> > Not sure why it was done this way, maybe to minimize code changes
> > while backporting.
> > Simplest fix is probably to add __maybe_unused to ioctls_out declaration.
>
> Unfortunately, that doesn't quite do it. That would fix the warning,
> but the code in the original e-mail is still wrong: we incorrectly
> don't report the _UFFDIO_CONTINUE bit, unless we use the value we
> computed in ioctls_out.

I think what happened is that Axel's patch [1] depends on the patch
from Peter [2]. [1] was backported without the dependency and the
conflict resolutions aren't quite correct.

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/

- Kalesh
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-19 22:02     ` Axel Rasmussen
  2022-01-19 22:10       ` Kalesh Singh
@ 2022-01-19 22:11       ` Lokesh Gidra
  2022-01-20 18:14         ` Axel Rasmussen
  1 sibling, 1 reply; 9+ messages in thread
From: Lokesh Gidra @ 2022-01-19 22:11 UTC (permalink / raw)
  To: Axel Rasmussen
  Cc: kbuild-all, kernel test robot, llvm, LKML, Peter Xu,
	Linux Memory Management List, Kalesh Singh, GNU/Weeb Mailing List,
	Andrew Morton, Suren Baghdasaryan, Todd Kjos

On Wed, Jan 19, 2022 at 2:03 PM Axel Rasmussen <[email protected]> wrote:
>
> On Wed, Jan 19, 2022 at 1:19 PM Suren Baghdasaryan <[email protected]> wrote:
> >
> > On Wed, Jan 19, 2022 at 12:39 PM Axel Rasmussen
> > <[email protected]> wrote:
> > >
> > > Lokesh, I only spent a few moments looking, but my suspicion is that
> > > this is the result of an incomplete backport of these patches to the
> > > Android kernel?
> > >
> > > The UFFDIO_CONTINUE patches introduced ioctls_out since the
> > > computation of it got more complicated, and those same patches also
> > > modified the put_user() on line 1533 here to use ioctls_out. I think
> > > Android backported the first part, but left out the second?
> >
> > The backport https://android-review.googlesource.com/c/kernel/common/+/1652718
> > looks like a mixture of
> > 14819305e09fe4 ("userfaultfd: wp: declare _UFFDIO_WRITEPROTECT
> > conditionally") and f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE
> > ioctl").
> > Not sure why it was done this way, maybe to minimize code changes
> > while backporting.
> > Simplest fix is probably to add __maybe_unused to ioctls_out declaration.
>
> Unfortunately, that doesn't quite do it. That would fix the warning,
> but the code in the original e-mail is still wrong: we incorrectly
> don't report the _UFFDIO_CONTINUE bit, unless we use the value we
> computed in ioctls_out.

Apologies for the screw up. Actually, since 5.4 kernel didn't already
have UFFD_WRITE_PROTECT feature and neither was I interested in it, so
I attempted to only backport UFFDIO_CONTINUE feature and there
mistakenly missed this one out.

Axel, wouldn't doing what Peter did in the following patch the right
thing to do?
https://lore.kernel.org/lkml/[email protected]/

Basically:

- if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
-     UFFD_API_RANGE_IOCTLS,
-     &user_uffdio_register->ioctls))
+ if (put_user(ioctls_out, &user_uffdio_register->ioctls))


>
> >
> > >
> > > On Sun, Jan 16, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
> > > >
> > > > Hi Axel,
> > > >
> > > > FYI, the error/warning still remains.
> > > >
> > > > tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
> > > > head:   bdf17ba628090156b539b1474eb5c636eeaf571b
> > > > commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
> > > > config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
> > > > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
> > > > reproduce (this is a W=1 build):
> > > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > >         chmod +x ~/bin/make.cross
> > > >         # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > >         git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
> > > >         git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
> > > >         git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > >         # save the config file to linux build tree
> > > >         mkdir build_dir
> > > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> > > >
> > > > If you fix the issue, kindly add following tag as appropriate
> > > > Reported-by: kernel test robot <[email protected]>
> > > >
> > > > All warnings (new ones prefixed by >>):
> > > >
> > > > >> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
> > > >                    __u64 ioctls_out;
> > > >                          ^
> > > >    1 warning generated.
> > > >
> > > >
> > > > vim +/ioctls_out +1519 fs/userfaultfd.c
> > > >
> > > >   1311
> > > >   1312  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > > >   1313                                  unsigned long arg)
> > > >   1314  {
> > > >   1315          struct mm_struct *mm = ctx->mm;
> > > >   1316          struct vm_area_struct *vma, *prev, *cur;
> > > >   1317          int ret;
> > > >   1318          struct uffdio_register uffdio_register;
> > > >   1319          struct uffdio_register __user *user_uffdio_register;
> > > >   1320          unsigned long vm_flags, new_flags;
> > > >   1321          bool found;
> > > >   1322          bool basic_ioctls;
> > > >   1323          unsigned long start, end, vma_end;
> > > >   1324
> > > >   1325          user_uffdio_register = (struct uffdio_register __user *) arg;
> > > >   1326
> > > >   1327          ret = -EFAULT;
> > > >   1328          if (copy_from_user(&uffdio_register, user_uffdio_register,
> > > >   1329                             sizeof(uffdio_register)-sizeof(__u64)))
> > > >   1330                  goto out;
> > > >   1331
> > > >   1332          ret = -EINVAL;
> > > >   1333          if (!uffdio_register.mode)
> > > >   1334                  goto out;
> > > >   1335          if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
> > > >   1336                  goto out;
> > > >   1337          vm_flags = 0;
> > > >   1338          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
> > > >   1339                  vm_flags |= VM_UFFD_MISSING;
> > > >   1340          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
> > > >   1341                  vm_flags |= VM_UFFD_WP;
> > > >   1342                  /*
> > > >   1343                   * FIXME: remove the below error constraint by
> > > >   1344                   * implementing the wprotect tracking mode.
> > > >   1345                   */
> > > >   1346                  ret = -EINVAL;
> > > >   1347                  goto out;
> > > >   1348          }
> > > >   1349          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
> > > >   1350  #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> > > >   1351                  goto out;
> > > >   1352  #endif
> > > >   1353                  vm_flags |= VM_UFFD_MINOR;
> > > >   1354          }
> > > >   1355
> > > >   1356          ret = validate_range(mm, &uffdio_register.range.start,
> > > >   1357                               uffdio_register.range.len);
> > > >   1358          if (ret)
> > > >   1359                  goto out;
> > > >   1360
> > > >   1361          start = uffdio_register.range.start;
> > > >   1362          end = start + uffdio_register.range.len;
> > > >   1363
> > > >   1364          ret = -ENOMEM;
> > > >   1365          if (!mmget_not_zero(mm))
> > > >   1366                  goto out;
> > > >   1367
> > > >   1368          down_write(&mm->mmap_sem);
> > > >   1369          if (!mmget_still_valid(mm))
> > > >   1370                  goto out_unlock;
> > > >   1371          vma = find_vma_prev(mm, start, &prev);
> > > >   1372          if (!vma)
> > > >   1373                  goto out_unlock;
> > > >   1374
> > > >   1375          /* check that there's at least one vma in the range */
> > > >   1376          ret = -EINVAL;
> > > >   1377          if (vma->vm_start >= end)
> > > >   1378                  goto out_unlock;
> > > >   1379
> > > >   1380          /*
> > > >   1381           * If the first vma contains huge pages, make sure start address
> > > >   1382           * is aligned to huge page size.
> > > >   1383           */
> > > >   1384          if (is_vm_hugetlb_page(vma)) {
> > > >   1385                  unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
> > > >   1386
> > > >   1387                  if (start & (vma_hpagesize - 1))
> > > >   1388                          goto out_unlock;
> > > >   1389          }
> > > >   1390
> > > >   1391          /*
> > > >   1392           * Search for not compatible vmas.
> > > >   1393           */
> > > >   1394          found = false;
> > > >   1395          basic_ioctls = false;
> > > >   1396          for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
> > > >   1397                  cond_resched();
> > > >   1398
> > > >   1399                  BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
> > > >   1400                         !!(cur->vm_flags & __VM_UFFD_FLAGS));
> > > >   1401
> > > >   1402                  /* check not compatible vmas */
> > > >   1403                  ret = -EINVAL;
> > > >   1404                  if (!vma_can_userfault(cur, vm_flags))
> > > >   1405                          goto out_unlock;
> > > >   1406
> > > >   1407                  /*
> > > >   1408                   * UFFDIO_COPY will fill file holes even without
> > > >   1409                   * PROT_WRITE. This check enforces that if this is a
> > > >   1410                   * MAP_SHARED, the process has write permission to the backing
> > > >   1411                   * file. If VM_MAYWRITE is set it also enforces that on a
> > > >   1412                   * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
> > > >   1413                   * F_WRITE_SEAL can be taken until the vma is destroyed.
> > > >   1414                   */
> > > >   1415                  ret = -EPERM;
> > > >   1416                  if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
> > > >   1417                          goto out_unlock;
> > > >   1418
> > > >   1419                  /*
> > > >   1420                   * If this vma contains ending address, and huge pages
> > > >   1421                   * check alignment.
> > > >   1422                   */
> > > >   1423                  if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
> > > >   1424                      end > cur->vm_start) {
> > > >   1425                          unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
> > > >   1426
> > > >   1427                          ret = -EINVAL;
> > > >   1428
> > > >   1429                          if (end & (vma_hpagesize - 1))
> > > >   1430                                  goto out_unlock;
> > > >   1431                  }
> > > >   1432
> > > >   1433                  /*
> > > >   1434                   * Check that this vma isn't already owned by a
> > > >   1435                   * different userfaultfd. We can't allow more than one
> > > >   1436                   * userfaultfd to own a single vma simultaneously or we
> > > >   1437                   * wouldn't know which one to deliver the userfaults to.
> > > >   1438                   */
> > > >   1439                  ret = -EBUSY;
> > > >   1440                  if (cur->vm_userfaultfd_ctx.ctx &&
> > > >   1441                      cur->vm_userfaultfd_ctx.ctx != ctx)
> > > >   1442                          goto out_unlock;
> > > >   1443
> > > >   1444                  /*
> > > >   1445                   * Note vmas containing huge pages
> > > >   1446                   */
> > > >   1447                  if (is_vm_hugetlb_page(cur))
> > > >   1448                          basic_ioctls = true;
> > > >   1449
> > > >   1450                  found = true;
> > > >   1451          }
> > > >   1452          BUG_ON(!found);
> > > >   1453
> > > >   1454          if (vma->vm_start < start)
> > > >   1455                  prev = vma;
> > > >   1456
> > > >   1457          ret = 0;
> > > >   1458          do {
> > > >   1459                  cond_resched();
> > > >   1460
> > > >   1461                  BUG_ON(!vma_can_userfault(vma, vm_flags));
> > > >   1462                  BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> > > >   1463                         vma->vm_userfaultfd_ctx.ctx != ctx);
> > > >   1464                  WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> > > >   1465
> > > >   1466                  /*
> > > >   1467                   * Nothing to do: this vma is already registered into this
> > > >   1468                   * userfaultfd and with the right tracking mode too.
> > > >   1469                   */
> > > >   1470                  if (vma->vm_userfaultfd_ctx.ctx == ctx &&
> > > >   1471                      (vma->vm_flags & vm_flags) == vm_flags)
> > > >   1472                          goto skip;
> > > >   1473
> > > >   1474                  if (vma->vm_start > start)
> > > >   1475                          start = vma->vm_start;
> > > >   1476                  vma_end = min(end, vma->vm_end);
> > > >   1477
> > > >   1478                  new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
> > > >   1479                  prev = vma_merge(mm, prev, start, vma_end, new_flags,
> > > >   1480                                   vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> > > >   1481                                   vma_policy(vma),
> > > >   1482                                   ((struct vm_userfaultfd_ctx){ ctx }),
> > > >   1483                                   vma_get_anon_name(vma));
> > > >   1484                  if (prev) {
> > > >   1485                          vma = prev;
> > > >   1486                          goto next;
> > > >   1487                  }
> > > >   1488                  if (vma->vm_start < start) {
> > > >   1489                          ret = split_vma(mm, vma, start, 1);
> > > >   1490                          if (ret)
> > > >   1491                                  break;
> > > >   1492                  }
> > > >   1493                  if (vma->vm_end > end) {
> > > >   1494                          ret = split_vma(mm, vma, end, 0);
> > > >   1495                          if (ret)
> > > >   1496                                  break;
> > > >   1497                  }
> > > >   1498          next:
> > > >   1499                  /*
> > > >   1500                   * In the vma_merge() successful mprotect-like case 8:
> > > >   1501                   * the next vma was merged into the current one and
> > > >   1502                   * the current one has not been updated yet.
> > > >   1503                   */
> > > >   1504                  vma->vm_flags = new_flags;
> > > >   1505                  vma->vm_userfaultfd_ctx.ctx = ctx;
> > > >   1506
> > > >   1507                  if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
> > > >   1508                          hugetlb_unshare_all_pmds(vma);
> > > >   1509
> > > >   1510          skip:
> > > >   1511                  prev = vma;
> > > >   1512                  start = vma->vm_end;
> > > >   1513                  vma = vma->vm_next;
> > > >   1514          } while (vma && vma->vm_start < end);
> > > >   1515  out_unlock:
> > > >   1516          up_write(&mm->mmap_sem);
> > > >   1517          mmput(mm);
> > > >   1518          if (!ret) {
> > > > > 1519                  __u64 ioctls_out;
> > > >   1520
> > > >   1521                  ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > >   1522                      UFFD_API_RANGE_IOCTLS;
> > > >   1523
> > > >   1524                  /* CONTINUE ioctl is only supported for MINOR ranges. */
> > > >   1525                  if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
> > > >   1526                          ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
> > > >   1527
> > > >   1528                  /*
> > > >   1529                   * Now that we scanned all vmas we can already tell
> > > >   1530                   * userland which ioctls methods are guaranteed to
> > > >   1531                   * succeed on this range.
> > > >   1532                   */
> > > >   1533                  if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > >   1534                               UFFD_API_RANGE_IOCTLS,
> > > >   1535                               &user_uffdio_register->ioctls))
> > > >   1536                          ret = -EFAULT;
> > > >   1537          }
> > > >   1538  out:
> > > >   1539          return ret;
> > > >   1540  }
> > > >   1541
> > > >
> > > > ---
> > > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > > https://lists.01.org/hyperkitty/list/[email protected]
> > >
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-19 22:11       ` Lokesh Gidra
@ 2022-01-20 18:14         ` Axel Rasmussen
  2022-01-20 18:16           ` Lokesh Gidra
  0 siblings, 1 reply; 9+ messages in thread
From: Axel Rasmussen @ 2022-01-20 18:14 UTC (permalink / raw)
  To: Lokesh Gidra
  Cc: kbuild-all, kernel test robot, llvm, LKML, Peter Xu,
	Linux Memory Management List, Kalesh Singh, GNU/Weeb Mailing List,
	Andrew Morton, Suren Baghdasaryan, Todd Kjos

On Wed, Jan 19, 2022 at 2:11 PM Lokesh Gidra <[email protected]> wrote:
>
> On Wed, Jan 19, 2022 at 2:03 PM Axel Rasmussen <[email protected]> wrote:
> >
> > On Wed, Jan 19, 2022 at 1:19 PM Suren Baghdasaryan <[email protected]> wrote:
> > >
> > > On Wed, Jan 19, 2022 at 12:39 PM Axel Rasmussen
> > > <[email protected]> wrote:
> > > >
> > > > Lokesh, I only spent a few moments looking, but my suspicion is that
> > > > this is the result of an incomplete backport of these patches to the
> > > > Android kernel?
> > > >
> > > > The UFFDIO_CONTINUE patches introduced ioctls_out since the
> > > > computation of it got more complicated, and those same patches also
> > > > modified the put_user() on line 1533 here to use ioctls_out. I think
> > > > Android backported the first part, but left out the second?
> > >
> > > The backport https://android-review.googlesource.com/c/kernel/common/+/1652718
> > > looks like a mixture of
> > > 14819305e09fe4 ("userfaultfd: wp: declare _UFFDIO_WRITEPROTECT
> > > conditionally") and f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE
> > > ioctl").
> > > Not sure why it was done this way, maybe to minimize code changes
> > > while backporting.
> > > Simplest fix is probably to add __maybe_unused to ioctls_out declaration.
> >
> > Unfortunately, that doesn't quite do it. That would fix the warning,
> > but the code in the original e-mail is still wrong: we incorrectly
> > don't report the _UFFDIO_CONTINUE bit, unless we use the value we
> > computed in ioctls_out.
>
> Apologies for the screw up. Actually, since 5.4 kernel didn't already
> have UFFD_WRITE_PROTECT feature and neither was I interested in it, so
> I attempted to only backport UFFDIO_CONTINUE feature and there
> mistakenly missed this one out.
>
> Axel, wouldn't doing what Peter did in the following patch the right
> thing to do?
> https://lore.kernel.org/lkml/[email protected]/
>
> Basically:
>
> - if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> -     UFFD_API_RANGE_IOCTLS,
> -     &user_uffdio_register->ioctls))
> + if (put_user(ioctls_out, &user_uffdio_register->ioctls))

Exactly right, that's the solution. And, I believe Kalesh has now
applied exactly that to the Android tree here:
https://android-review.googlesource.com/c/kernel/common/+/1955138 - so
once that's merged, we can consider this fixed.

>
>
> >
> > >
> > > >
> > > > On Sun, Jan 16, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
> > > > >
> > > > > Hi Axel,
> > > > >
> > > > > FYI, the error/warning still remains.
> > > > >
> > > > > tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
> > > > > head:   bdf17ba628090156b539b1474eb5c636eeaf571b
> > > > > commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
> > > > > config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
> > > > > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
> > > > > reproduce (this is a W=1 build):
> > > > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > > >         chmod +x ~/bin/make.cross
> > > > >         # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > > >         git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
> > > > >         git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
> > > > >         git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > > >         # save the config file to linux build tree
> > > > >         mkdir build_dir
> > > > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> > > > >
> > > > > If you fix the issue, kindly add following tag as appropriate
> > > > > Reported-by: kernel test robot <[email protected]>
> > > > >
> > > > > All warnings (new ones prefixed by >>):
> > > > >
> > > > > >> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
> > > > >                    __u64 ioctls_out;
> > > > >                          ^
> > > > >    1 warning generated.
> > > > >
> > > > >
> > > > > vim +/ioctls_out +1519 fs/userfaultfd.c
> > > > >
> > > > >   1311
> > > > >   1312  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > > > >   1313                                  unsigned long arg)
> > > > >   1314  {
> > > > >   1315          struct mm_struct *mm = ctx->mm;
> > > > >   1316          struct vm_area_struct *vma, *prev, *cur;
> > > > >   1317          int ret;
> > > > >   1318          struct uffdio_register uffdio_register;
> > > > >   1319          struct uffdio_register __user *user_uffdio_register;
> > > > >   1320          unsigned long vm_flags, new_flags;
> > > > >   1321          bool found;
> > > > >   1322          bool basic_ioctls;
> > > > >   1323          unsigned long start, end, vma_end;
> > > > >   1324
> > > > >   1325          user_uffdio_register = (struct uffdio_register __user *) arg;
> > > > >   1326
> > > > >   1327          ret = -EFAULT;
> > > > >   1328          if (copy_from_user(&uffdio_register, user_uffdio_register,
> > > > >   1329                             sizeof(uffdio_register)-sizeof(__u64)))
> > > > >   1330                  goto out;
> > > > >   1331
> > > > >   1332          ret = -EINVAL;
> > > > >   1333          if (!uffdio_register.mode)
> > > > >   1334                  goto out;
> > > > >   1335          if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
> > > > >   1336                  goto out;
> > > > >   1337          vm_flags = 0;
> > > > >   1338          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
> > > > >   1339                  vm_flags |= VM_UFFD_MISSING;
> > > > >   1340          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
> > > > >   1341                  vm_flags |= VM_UFFD_WP;
> > > > >   1342                  /*
> > > > >   1343                   * FIXME: remove the below error constraint by
> > > > >   1344                   * implementing the wprotect tracking mode.
> > > > >   1345                   */
> > > > >   1346                  ret = -EINVAL;
> > > > >   1347                  goto out;
> > > > >   1348          }
> > > > >   1349          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
> > > > >   1350  #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> > > > >   1351                  goto out;
> > > > >   1352  #endif
> > > > >   1353                  vm_flags |= VM_UFFD_MINOR;
> > > > >   1354          }
> > > > >   1355
> > > > >   1356          ret = validate_range(mm, &uffdio_register.range.start,
> > > > >   1357                               uffdio_register.range.len);
> > > > >   1358          if (ret)
> > > > >   1359                  goto out;
> > > > >   1360
> > > > >   1361          start = uffdio_register.range.start;
> > > > >   1362          end = start + uffdio_register.range.len;
> > > > >   1363
> > > > >   1364          ret = -ENOMEM;
> > > > >   1365          if (!mmget_not_zero(mm))
> > > > >   1366                  goto out;
> > > > >   1367
> > > > >   1368          down_write(&mm->mmap_sem);
> > > > >   1369          if (!mmget_still_valid(mm))
> > > > >   1370                  goto out_unlock;
> > > > >   1371          vma = find_vma_prev(mm, start, &prev);
> > > > >   1372          if (!vma)
> > > > >   1373                  goto out_unlock;
> > > > >   1374
> > > > >   1375          /* check that there's at least one vma in the range */
> > > > >   1376          ret = -EINVAL;
> > > > >   1377          if (vma->vm_start >= end)
> > > > >   1378                  goto out_unlock;
> > > > >   1379
> > > > >   1380          /*
> > > > >   1381           * If the first vma contains huge pages, make sure start address
> > > > >   1382           * is aligned to huge page size.
> > > > >   1383           */
> > > > >   1384          if (is_vm_hugetlb_page(vma)) {
> > > > >   1385                  unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
> > > > >   1386
> > > > >   1387                  if (start & (vma_hpagesize - 1))
> > > > >   1388                          goto out_unlock;
> > > > >   1389          }
> > > > >   1390
> > > > >   1391          /*
> > > > >   1392           * Search for not compatible vmas.
> > > > >   1393           */
> > > > >   1394          found = false;
> > > > >   1395          basic_ioctls = false;
> > > > >   1396          for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
> > > > >   1397                  cond_resched();
> > > > >   1398
> > > > >   1399                  BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
> > > > >   1400                         !!(cur->vm_flags & __VM_UFFD_FLAGS));
> > > > >   1401
> > > > >   1402                  /* check not compatible vmas */
> > > > >   1403                  ret = -EINVAL;
> > > > >   1404                  if (!vma_can_userfault(cur, vm_flags))
> > > > >   1405                          goto out_unlock;
> > > > >   1406
> > > > >   1407                  /*
> > > > >   1408                   * UFFDIO_COPY will fill file holes even without
> > > > >   1409                   * PROT_WRITE. This check enforces that if this is a
> > > > >   1410                   * MAP_SHARED, the process has write permission to the backing
> > > > >   1411                   * file. If VM_MAYWRITE is set it also enforces that on a
> > > > >   1412                   * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
> > > > >   1413                   * F_WRITE_SEAL can be taken until the vma is destroyed.
> > > > >   1414                   */
> > > > >   1415                  ret = -EPERM;
> > > > >   1416                  if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
> > > > >   1417                          goto out_unlock;
> > > > >   1418
> > > > >   1419                  /*
> > > > >   1420                   * If this vma contains ending address, and huge pages
> > > > >   1421                   * check alignment.
> > > > >   1422                   */
> > > > >   1423                  if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
> > > > >   1424                      end > cur->vm_start) {
> > > > >   1425                          unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
> > > > >   1426
> > > > >   1427                          ret = -EINVAL;
> > > > >   1428
> > > > >   1429                          if (end & (vma_hpagesize - 1))
> > > > >   1430                                  goto out_unlock;
> > > > >   1431                  }
> > > > >   1432
> > > > >   1433                  /*
> > > > >   1434                   * Check that this vma isn't already owned by a
> > > > >   1435                   * different userfaultfd. We can't allow more than one
> > > > >   1436                   * userfaultfd to own a single vma simultaneously or we
> > > > >   1437                   * wouldn't know which one to deliver the userfaults to.
> > > > >   1438                   */
> > > > >   1439                  ret = -EBUSY;
> > > > >   1440                  if (cur->vm_userfaultfd_ctx.ctx &&
> > > > >   1441                      cur->vm_userfaultfd_ctx.ctx != ctx)
> > > > >   1442                          goto out_unlock;
> > > > >   1443
> > > > >   1444                  /*
> > > > >   1445                   * Note vmas containing huge pages
> > > > >   1446                   */
> > > > >   1447                  if (is_vm_hugetlb_page(cur))
> > > > >   1448                          basic_ioctls = true;
> > > > >   1449
> > > > >   1450                  found = true;
> > > > >   1451          }
> > > > >   1452          BUG_ON(!found);
> > > > >   1453
> > > > >   1454          if (vma->vm_start < start)
> > > > >   1455                  prev = vma;
> > > > >   1456
> > > > >   1457          ret = 0;
> > > > >   1458          do {
> > > > >   1459                  cond_resched();
> > > > >   1460
> > > > >   1461                  BUG_ON(!vma_can_userfault(vma, vm_flags));
> > > > >   1462                  BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> > > > >   1463                         vma->vm_userfaultfd_ctx.ctx != ctx);
> > > > >   1464                  WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> > > > >   1465
> > > > >   1466                  /*
> > > > >   1467                   * Nothing to do: this vma is already registered into this
> > > > >   1468                   * userfaultfd and with the right tracking mode too.
> > > > >   1469                   */
> > > > >   1470                  if (vma->vm_userfaultfd_ctx.ctx == ctx &&
> > > > >   1471                      (vma->vm_flags & vm_flags) == vm_flags)
> > > > >   1472                          goto skip;
> > > > >   1473
> > > > >   1474                  if (vma->vm_start > start)
> > > > >   1475                          start = vma->vm_start;
> > > > >   1476                  vma_end = min(end, vma->vm_end);
> > > > >   1477
> > > > >   1478                  new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
> > > > >   1479                  prev = vma_merge(mm, prev, start, vma_end, new_flags,
> > > > >   1480                                   vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> > > > >   1481                                   vma_policy(vma),
> > > > >   1482                                   ((struct vm_userfaultfd_ctx){ ctx }),
> > > > >   1483                                   vma_get_anon_name(vma));
> > > > >   1484                  if (prev) {
> > > > >   1485                          vma = prev;
> > > > >   1486                          goto next;
> > > > >   1487                  }
> > > > >   1488                  if (vma->vm_start < start) {
> > > > >   1489                          ret = split_vma(mm, vma, start, 1);
> > > > >   1490                          if (ret)
> > > > >   1491                                  break;
> > > > >   1492                  }
> > > > >   1493                  if (vma->vm_end > end) {
> > > > >   1494                          ret = split_vma(mm, vma, end, 0);
> > > > >   1495                          if (ret)
> > > > >   1496                                  break;
> > > > >   1497                  }
> > > > >   1498          next:
> > > > >   1499                  /*
> > > > >   1500                   * In the vma_merge() successful mprotect-like case 8:
> > > > >   1501                   * the next vma was merged into the current one and
> > > > >   1502                   * the current one has not been updated yet.
> > > > >   1503                   */
> > > > >   1504                  vma->vm_flags = new_flags;
> > > > >   1505                  vma->vm_userfaultfd_ctx.ctx = ctx;
> > > > >   1506
> > > > >   1507                  if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
> > > > >   1508                          hugetlb_unshare_all_pmds(vma);
> > > > >   1509
> > > > >   1510          skip:
> > > > >   1511                  prev = vma;
> > > > >   1512                  start = vma->vm_end;
> > > > >   1513                  vma = vma->vm_next;
> > > > >   1514          } while (vma && vma->vm_start < end);
> > > > >   1515  out_unlock:
> > > > >   1516          up_write(&mm->mmap_sem);
> > > > >   1517          mmput(mm);
> > > > >   1518          if (!ret) {
> > > > > > 1519                  __u64 ioctls_out;
> > > > >   1520
> > > > >   1521                  ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > > >   1522                      UFFD_API_RANGE_IOCTLS;
> > > > >   1523
> > > > >   1524                  /* CONTINUE ioctl is only supported for MINOR ranges. */
> > > > >   1525                  if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
> > > > >   1526                          ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
> > > > >   1527
> > > > >   1528                  /*
> > > > >   1529                   * Now that we scanned all vmas we can already tell
> > > > >   1530                   * userland which ioctls methods are guaranteed to
> > > > >   1531                   * succeed on this range.
> > > > >   1532                   */
> > > > >   1533                  if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > > >   1534                               UFFD_API_RANGE_IOCTLS,
> > > > >   1535                               &user_uffdio_register->ioctls))
> > > > >   1536                          ret = -EFAULT;
> > > > >   1537          }
> > > > >   1538  out:
> > > > >   1539          return ret;
> > > > >   1540  }
> > > > >   1541
> > > > >
> > > > > ---
> > > > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > > > https://lists.01.org/hyperkitty/list/[email protected]
> > > >
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-20 18:14         ` Axel Rasmussen
@ 2022-01-20 18:16           ` Lokesh Gidra
  2022-01-20 18:30             ` Axel Rasmussen
  0 siblings, 1 reply; 9+ messages in thread
From: Lokesh Gidra @ 2022-01-20 18:16 UTC (permalink / raw)
  To: Axel Rasmussen
  Cc: kbuild-all, kernel test robot, llvm, LKML, Peter Xu,
	Linux Memory Management List, Kalesh Singh, GNU/Weeb Mailing List,
	Andrew Morton, Suren Baghdasaryan, Todd Kjos

Thanks a lot for confirming. I hope I have not screwed up like this
anywhere else.

On Thu, Jan 20, 2022 at 10:15 AM Axel Rasmussen
<[email protected]> wrote:
>
> On Wed, Jan 19, 2022 at 2:11 PM Lokesh Gidra <[email protected]> wrote:
> >
> > On Wed, Jan 19, 2022 at 2:03 PM Axel Rasmussen <[email protected]> wrote:
> > >
> > > On Wed, Jan 19, 2022 at 1:19 PM Suren Baghdasaryan <[email protected]> wrote:
> > > >
> > > > On Wed, Jan 19, 2022 at 12:39 PM Axel Rasmussen
> > > > <[email protected]> wrote:
> > > > >
> > > > > Lokesh, I only spent a few moments looking, but my suspicion is that
> > > > > this is the result of an incomplete backport of these patches to the
> > > > > Android kernel?
> > > > >
> > > > > The UFFDIO_CONTINUE patches introduced ioctls_out since the
> > > > > computation of it got more complicated, and those same patches also
> > > > > modified the put_user() on line 1533 here to use ioctls_out. I think
> > > > > Android backported the first part, but left out the second?
> > > >
> > > > The backport https://android-review.googlesource.com/c/kernel/common/+/1652718
> > > > looks like a mixture of
> > > > 14819305e09fe4 ("userfaultfd: wp: declare _UFFDIO_WRITEPROTECT
> > > > conditionally") and f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE
> > > > ioctl").
> > > > Not sure why it was done this way, maybe to minimize code changes
> > > > while backporting.
> > > > Simplest fix is probably to add __maybe_unused to ioctls_out declaration.
> > >
> > > Unfortunately, that doesn't quite do it. That would fix the warning,
> > > but the code in the original e-mail is still wrong: we incorrectly
> > > don't report the _UFFDIO_CONTINUE bit, unless we use the value we
> > > computed in ioctls_out.
> >
> > Apologies for the screw up. Actually, since 5.4 kernel didn't already
> > have UFFD_WRITE_PROTECT feature and neither was I interested in it, so
> > I attempted to only backport UFFDIO_CONTINUE feature and there
> > mistakenly missed this one out.
> >
> > Axel, wouldn't doing what Peter did in the following patch the right
> > thing to do?
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > Basically:
> >
> > - if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > -     UFFD_API_RANGE_IOCTLS,
> > -     &user_uffdio_register->ioctls))
> > + if (put_user(ioctls_out, &user_uffdio_register->ioctls))
>
> Exactly right, that's the solution. And, I believe Kalesh has now
> applied exactly that to the Android tree here:
> https://android-review.googlesource.com/c/kernel/common/+/1955138 - so
> once that's merged, we can consider this fixed.
>
> >
> >
> > >
> > > >
> > > > >
> > > > > On Sun, Jan 16, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
> > > > > >
> > > > > > Hi Axel,
> > > > > >
> > > > > > FYI, the error/warning still remains.
> > > > > >
> > > > > > tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
> > > > > > head:   bdf17ba628090156b539b1474eb5c636eeaf571b
> > > > > > commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
> > > > > > config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
> > > > > > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
> > > > > > reproduce (this is a W=1 build):
> > > > > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > > > >         chmod +x ~/bin/make.cross
> > > > > >         # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > > > >         git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
> > > > > >         git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
> > > > > >         git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > > > >         # save the config file to linux build tree
> > > > > >         mkdir build_dir
> > > > > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> > > > > >
> > > > > > If you fix the issue, kindly add following tag as appropriate
> > > > > > Reported-by: kernel test robot <[email protected]>
> > > > > >
> > > > > > All warnings (new ones prefixed by >>):
> > > > > >
> > > > > > >> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
> > > > > >                    __u64 ioctls_out;
> > > > > >                          ^
> > > > > >    1 warning generated.
> > > > > >
> > > > > >
> > > > > > vim +/ioctls_out +1519 fs/userfaultfd.c
> > > > > >
> > > > > >   1311
> > > > > >   1312  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > > > > >   1313                                  unsigned long arg)
> > > > > >   1314  {
> > > > > >   1315          struct mm_struct *mm = ctx->mm;
> > > > > >   1316          struct vm_area_struct *vma, *prev, *cur;
> > > > > >   1317          int ret;
> > > > > >   1318          struct uffdio_register uffdio_register;
> > > > > >   1319          struct uffdio_register __user *user_uffdio_register;
> > > > > >   1320          unsigned long vm_flags, new_flags;
> > > > > >   1321          bool found;
> > > > > >   1322          bool basic_ioctls;
> > > > > >   1323          unsigned long start, end, vma_end;
> > > > > >   1324
> > > > > >   1325          user_uffdio_register = (struct uffdio_register __user *) arg;
> > > > > >   1326
> > > > > >   1327          ret = -EFAULT;
> > > > > >   1328          if (copy_from_user(&uffdio_register, user_uffdio_register,
> > > > > >   1329                             sizeof(uffdio_register)-sizeof(__u64)))
> > > > > >   1330                  goto out;
> > > > > >   1331
> > > > > >   1332          ret = -EINVAL;
> > > > > >   1333          if (!uffdio_register.mode)
> > > > > >   1334                  goto out;
> > > > > >   1335          if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
> > > > > >   1336                  goto out;
> > > > > >   1337          vm_flags = 0;
> > > > > >   1338          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
> > > > > >   1339                  vm_flags |= VM_UFFD_MISSING;
> > > > > >   1340          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
> > > > > >   1341                  vm_flags |= VM_UFFD_WP;
> > > > > >   1342                  /*
> > > > > >   1343                   * FIXME: remove the below error constraint by
> > > > > >   1344                   * implementing the wprotect tracking mode.
> > > > > >   1345                   */
> > > > > >   1346                  ret = -EINVAL;
> > > > > >   1347                  goto out;
> > > > > >   1348          }
> > > > > >   1349          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
> > > > > >   1350  #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> > > > > >   1351                  goto out;
> > > > > >   1352  #endif
> > > > > >   1353                  vm_flags |= VM_UFFD_MINOR;
> > > > > >   1354          }
> > > > > >   1355
> > > > > >   1356          ret = validate_range(mm, &uffdio_register.range.start,
> > > > > >   1357                               uffdio_register.range.len);
> > > > > >   1358          if (ret)
> > > > > >   1359                  goto out;
> > > > > >   1360
> > > > > >   1361          start = uffdio_register.range.start;
> > > > > >   1362          end = start + uffdio_register.range.len;
> > > > > >   1363
> > > > > >   1364          ret = -ENOMEM;
> > > > > >   1365          if (!mmget_not_zero(mm))
> > > > > >   1366                  goto out;
> > > > > >   1367
> > > > > >   1368          down_write(&mm->mmap_sem);
> > > > > >   1369          if (!mmget_still_valid(mm))
> > > > > >   1370                  goto out_unlock;
> > > > > >   1371          vma = find_vma_prev(mm, start, &prev);
> > > > > >   1372          if (!vma)
> > > > > >   1373                  goto out_unlock;
> > > > > >   1374
> > > > > >   1375          /* check that there's at least one vma in the range */
> > > > > >   1376          ret = -EINVAL;
> > > > > >   1377          if (vma->vm_start >= end)
> > > > > >   1378                  goto out_unlock;
> > > > > >   1379
> > > > > >   1380          /*
> > > > > >   1381           * If the first vma contains huge pages, make sure start address
> > > > > >   1382           * is aligned to huge page size.
> > > > > >   1383           */
> > > > > >   1384          if (is_vm_hugetlb_page(vma)) {
> > > > > >   1385                  unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
> > > > > >   1386
> > > > > >   1387                  if (start & (vma_hpagesize - 1))
> > > > > >   1388                          goto out_unlock;
> > > > > >   1389          }
> > > > > >   1390
> > > > > >   1391          /*
> > > > > >   1392           * Search for not compatible vmas.
> > > > > >   1393           */
> > > > > >   1394          found = false;
> > > > > >   1395          basic_ioctls = false;
> > > > > >   1396          for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
> > > > > >   1397                  cond_resched();
> > > > > >   1398
> > > > > >   1399                  BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
> > > > > >   1400                         !!(cur->vm_flags & __VM_UFFD_FLAGS));
> > > > > >   1401
> > > > > >   1402                  /* check not compatible vmas */
> > > > > >   1403                  ret = -EINVAL;
> > > > > >   1404                  if (!vma_can_userfault(cur, vm_flags))
> > > > > >   1405                          goto out_unlock;
> > > > > >   1406
> > > > > >   1407                  /*
> > > > > >   1408                   * UFFDIO_COPY will fill file holes even without
> > > > > >   1409                   * PROT_WRITE. This check enforces that if this is a
> > > > > >   1410                   * MAP_SHARED, the process has write permission to the backing
> > > > > >   1411                   * file. If VM_MAYWRITE is set it also enforces that on a
> > > > > >   1412                   * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
> > > > > >   1413                   * F_WRITE_SEAL can be taken until the vma is destroyed.
> > > > > >   1414                   */
> > > > > >   1415                  ret = -EPERM;
> > > > > >   1416                  if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
> > > > > >   1417                          goto out_unlock;
> > > > > >   1418
> > > > > >   1419                  /*
> > > > > >   1420                   * If this vma contains ending address, and huge pages
> > > > > >   1421                   * check alignment.
> > > > > >   1422                   */
> > > > > >   1423                  if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
> > > > > >   1424                      end > cur->vm_start) {
> > > > > >   1425                          unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
> > > > > >   1426
> > > > > >   1427                          ret = -EINVAL;
> > > > > >   1428
> > > > > >   1429                          if (end & (vma_hpagesize - 1))
> > > > > >   1430                                  goto out_unlock;
> > > > > >   1431                  }
> > > > > >   1432
> > > > > >   1433                  /*
> > > > > >   1434                   * Check that this vma isn't already owned by a
> > > > > >   1435                   * different userfaultfd. We can't allow more than one
> > > > > >   1436                   * userfaultfd to own a single vma simultaneously or we
> > > > > >   1437                   * wouldn't know which one to deliver the userfaults to.
> > > > > >   1438                   */
> > > > > >   1439                  ret = -EBUSY;
> > > > > >   1440                  if (cur->vm_userfaultfd_ctx.ctx &&
> > > > > >   1441                      cur->vm_userfaultfd_ctx.ctx != ctx)
> > > > > >   1442                          goto out_unlock;
> > > > > >   1443
> > > > > >   1444                  /*
> > > > > >   1445                   * Note vmas containing huge pages
> > > > > >   1446                   */
> > > > > >   1447                  if (is_vm_hugetlb_page(cur))
> > > > > >   1448                          basic_ioctls = true;
> > > > > >   1449
> > > > > >   1450                  found = true;
> > > > > >   1451          }
> > > > > >   1452          BUG_ON(!found);
> > > > > >   1453
> > > > > >   1454          if (vma->vm_start < start)
> > > > > >   1455                  prev = vma;
> > > > > >   1456
> > > > > >   1457          ret = 0;
> > > > > >   1458          do {
> > > > > >   1459                  cond_resched();
> > > > > >   1460
> > > > > >   1461                  BUG_ON(!vma_can_userfault(vma, vm_flags));
> > > > > >   1462                  BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> > > > > >   1463                         vma->vm_userfaultfd_ctx.ctx != ctx);
> > > > > >   1464                  WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> > > > > >   1465
> > > > > >   1466                  /*
> > > > > >   1467                   * Nothing to do: this vma is already registered into this
> > > > > >   1468                   * userfaultfd and with the right tracking mode too.
> > > > > >   1469                   */
> > > > > >   1470                  if (vma->vm_userfaultfd_ctx.ctx == ctx &&
> > > > > >   1471                      (vma->vm_flags & vm_flags) == vm_flags)
> > > > > >   1472                          goto skip;
> > > > > >   1473
> > > > > >   1474                  if (vma->vm_start > start)
> > > > > >   1475                          start = vma->vm_start;
> > > > > >   1476                  vma_end = min(end, vma->vm_end);
> > > > > >   1477
> > > > > >   1478                  new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
> > > > > >   1479                  prev = vma_merge(mm, prev, start, vma_end, new_flags,
> > > > > >   1480                                   vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> > > > > >   1481                                   vma_policy(vma),
> > > > > >   1482                                   ((struct vm_userfaultfd_ctx){ ctx }),
> > > > > >   1483                                   vma_get_anon_name(vma));
> > > > > >   1484                  if (prev) {
> > > > > >   1485                          vma = prev;
> > > > > >   1486                          goto next;
> > > > > >   1487                  }
> > > > > >   1488                  if (vma->vm_start < start) {
> > > > > >   1489                          ret = split_vma(mm, vma, start, 1);
> > > > > >   1490                          if (ret)
> > > > > >   1491                                  break;
> > > > > >   1492                  }
> > > > > >   1493                  if (vma->vm_end > end) {
> > > > > >   1494                          ret = split_vma(mm, vma, end, 0);
> > > > > >   1495                          if (ret)
> > > > > >   1496                                  break;
> > > > > >   1497                  }
> > > > > >   1498          next:
> > > > > >   1499                  /*
> > > > > >   1500                   * In the vma_merge() successful mprotect-like case 8:
> > > > > >   1501                   * the next vma was merged into the current one and
> > > > > >   1502                   * the current one has not been updated yet.
> > > > > >   1503                   */
> > > > > >   1504                  vma->vm_flags = new_flags;
> > > > > >   1505                  vma->vm_userfaultfd_ctx.ctx = ctx;
> > > > > >   1506
> > > > > >   1507                  if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
> > > > > >   1508                          hugetlb_unshare_all_pmds(vma);
> > > > > >   1509
> > > > > >   1510          skip:
> > > > > >   1511                  prev = vma;
> > > > > >   1512                  start = vma->vm_end;
> > > > > >   1513                  vma = vma->vm_next;
> > > > > >   1514          } while (vma && vma->vm_start < end);
> > > > > >   1515  out_unlock:
> > > > > >   1516          up_write(&mm->mmap_sem);
> > > > > >   1517          mmput(mm);
> > > > > >   1518          if (!ret) {
> > > > > > > 1519                  __u64 ioctls_out;
> > > > > >   1520
> > > > > >   1521                  ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > > > >   1522                      UFFD_API_RANGE_IOCTLS;
> > > > > >   1523
> > > > > >   1524                  /* CONTINUE ioctl is only supported for MINOR ranges. */
> > > > > >   1525                  if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
> > > > > >   1526                          ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
> > > > > >   1527
> > > > > >   1528                  /*
> > > > > >   1529                   * Now that we scanned all vmas we can already tell
> > > > > >   1530                   * userland which ioctls methods are guaranteed to
> > > > > >   1531                   * succeed on this range.
> > > > > >   1532                   */
> > > > > >   1533                  if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > > > >   1534                               UFFD_API_RANGE_IOCTLS,
> > > > > >   1535                               &user_uffdio_register->ioctls))
> > > > > >   1536                          ret = -EFAULT;
> > > > > >   1537          }
> > > > > >   1538  out:
> > > > > >   1539          return ret;
> > > > > >   1540  }
> > > > > >   1541
> > > > > >
> > > > > > ---
> > > > > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > > > > https://lists.01.org/hyperkitty/list/[email protected]
> > > > >
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used
  2022-01-20 18:16           ` Lokesh Gidra
@ 2022-01-20 18:30             ` Axel Rasmussen
  0 siblings, 0 replies; 9+ messages in thread
From: Axel Rasmussen @ 2022-01-20 18:30 UTC (permalink / raw)
  To: Lokesh Gidra
  Cc: kbuild-all, kernel test robot, llvm, LKML, Peter Xu,
	Linux Memory Management List, Kalesh Singh, GNU/Weeb Mailing List,
	Andrew Morton, Suren Baghdasaryan, Todd Kjos

On Thu, Jan 20, 2022 at 10:16 AM Lokesh Gidra <[email protected]> wrote:
>
> Thanks a lot for confirming. I hope I have not screwed up like this
> anywhere else.

No worries!

For what it's worth, I don't think this is *that* bad of a bug for
Android. It would mean, if we were using userfaultfd's feature
detection, we might mistakenly think UFFDIO_CONTINUE was not
available. But since Android has more direct control over userspace +
kernel than arbitrary Linux systems, I suspect your userspace code
doesn't really rely on the feature detection userfaultfd itself
provides? So, it may have not actually mattered to us in practice
anyway.

Still better to be fully correct. ;) But, could have been worse.

>
> On Thu, Jan 20, 2022 at 10:15 AM Axel Rasmussen
> <[email protected]> wrote:
> >
> > On Wed, Jan 19, 2022 at 2:11 PM Lokesh Gidra <[email protected]> wrote:
> > >
> > > On Wed, Jan 19, 2022 at 2:03 PM Axel Rasmussen <[email protected]> wrote:
> > > >
> > > > On Wed, Jan 19, 2022 at 1:19 PM Suren Baghdasaryan <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jan 19, 2022 at 12:39 PM Axel Rasmussen
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Lokesh, I only spent a few moments looking, but my suspicion is that
> > > > > > this is the result of an incomplete backport of these patches to the
> > > > > > Android kernel?
> > > > > >
> > > > > > The UFFDIO_CONTINUE patches introduced ioctls_out since the
> > > > > > computation of it got more complicated, and those same patches also
> > > > > > modified the put_user() on line 1533 here to use ioctls_out. I think
> > > > > > Android backported the first part, but left out the second?
> > > > >
> > > > > The backport https://android-review.googlesource.com/c/kernel/common/+/1652718
> > > > > looks like a mixture of
> > > > > 14819305e09fe4 ("userfaultfd: wp: declare _UFFDIO_WRITEPROTECT
> > > > > conditionally") and f619147104c8 ("userfaultfd: add UFFDIO_CONTINUE
> > > > > ioctl").
> > > > > Not sure why it was done this way, maybe to minimize code changes
> > > > > while backporting.
> > > > > Simplest fix is probably to add __maybe_unused to ioctls_out declaration.
> > > >
> > > > Unfortunately, that doesn't quite do it. That would fix the warning,
> > > > but the code in the original e-mail is still wrong: we incorrectly
> > > > don't report the _UFFDIO_CONTINUE bit, unless we use the value we
> > > > computed in ioctls_out.
> > >
> > > Apologies for the screw up. Actually, since 5.4 kernel didn't already
> > > have UFFD_WRITE_PROTECT feature and neither was I interested in it, so
> > > I attempted to only backport UFFDIO_CONTINUE feature and there
> > > mistakenly missed this one out.
> > >
> > > Axel, wouldn't doing what Peter did in the following patch the right
> > > thing to do?
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > Basically:
> > >
> > > - if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > -     UFFD_API_RANGE_IOCTLS,
> > > -     &user_uffdio_register->ioctls))
> > > + if (put_user(ioctls_out, &user_uffdio_register->ioctls))
> >
> > Exactly right, that's the solution. And, I believe Kalesh has now
> > applied exactly that to the Android tree here:
> > https://android-review.googlesource.com/c/kernel/common/+/1955138 - so
> > once that's merged, we can consider this fixed.
> >
> > >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > On Sun, Jan 16, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
> > > > > > >
> > > > > > > Hi Axel,
> > > > > > >
> > > > > > > FYI, the error/warning still remains.
> > > > > > >
> > > > > > > tree:   https://github.com/ammarfaizi2/linux-block google/android/kernel/common/android12-5.4
> > > > > > > head:   bdf17ba628090156b539b1474eb5c636eeaf571b
> > > > > > > commit: b69f713e60d03ae448e5c9fd92e5b0b193dea7be [6517/9999] BACKPORT: FROMGIT: userfaultfd: add UFFDIO_CONTINUE ioctl
> > > > > > > config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220117/[email protected]/config)
> > > > > > > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
> > > > > > > reproduce (this is a W=1 build):
> > > > > > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > > > > >         chmod +x ~/bin/make.cross
> > > > > > >         # https://github.com/ammarfaizi2/linux-block/commit/b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > > > > >         git remote add ammarfaizi2-block https://github.com/ammarfaizi2/linux-block
> > > > > > >         git fetch --no-tags ammarfaizi2-block google/android/kernel/common/android12-5.4
> > > > > > >         git checkout b69f713e60d03ae448e5c9fd92e5b0b193dea7be
> > > > > > >         # save the config file to linux build tree
> > > > > > >         mkdir build_dir
> > > > > > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> > > > > > >
> > > > > > > If you fix the issue, kindly add following tag as appropriate
> > > > > > > Reported-by: kernel test robot <[email protected]>
> > > > > > >
> > > > > > > All warnings (new ones prefixed by >>):
> > > > > > >
> > > > > > > >> fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used [-Wunused-but-set-variable]
> > > > > > >                    __u64 ioctls_out;
> > > > > > >                          ^
> > > > > > >    1 warning generated.
> > > > > > >
> > > > > > >
> > > > > > > vim +/ioctls_out +1519 fs/userfaultfd.c
> > > > > > >
> > > > > > >   1311
> > > > > > >   1312  static int userfaultfd_register(struct userfaultfd_ctx *ctx,
> > > > > > >   1313                                  unsigned long arg)
> > > > > > >   1314  {
> > > > > > >   1315          struct mm_struct *mm = ctx->mm;
> > > > > > >   1316          struct vm_area_struct *vma, *prev, *cur;
> > > > > > >   1317          int ret;
> > > > > > >   1318          struct uffdio_register uffdio_register;
> > > > > > >   1319          struct uffdio_register __user *user_uffdio_register;
> > > > > > >   1320          unsigned long vm_flags, new_flags;
> > > > > > >   1321          bool found;
> > > > > > >   1322          bool basic_ioctls;
> > > > > > >   1323          unsigned long start, end, vma_end;
> > > > > > >   1324
> > > > > > >   1325          user_uffdio_register = (struct uffdio_register __user *) arg;
> > > > > > >   1326
> > > > > > >   1327          ret = -EFAULT;
> > > > > > >   1328          if (copy_from_user(&uffdio_register, user_uffdio_register,
> > > > > > >   1329                             sizeof(uffdio_register)-sizeof(__u64)))
> > > > > > >   1330                  goto out;
> > > > > > >   1331
> > > > > > >   1332          ret = -EINVAL;
> > > > > > >   1333          if (!uffdio_register.mode)
> > > > > > >   1334                  goto out;
> > > > > > >   1335          if (uffdio_register.mode & ~UFFD_API_REGISTER_MODES)
> > > > > > >   1336                  goto out;
> > > > > > >   1337          vm_flags = 0;
> > > > > > >   1338          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MISSING)
> > > > > > >   1339                  vm_flags |= VM_UFFD_MISSING;
> > > > > > >   1340          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_WP) {
> > > > > > >   1341                  vm_flags |= VM_UFFD_WP;
> > > > > > >   1342                  /*
> > > > > > >   1343                   * FIXME: remove the below error constraint by
> > > > > > >   1344                   * implementing the wprotect tracking mode.
> > > > > > >   1345                   */
> > > > > > >   1346                  ret = -EINVAL;
> > > > > > >   1347                  goto out;
> > > > > > >   1348          }
> > > > > > >   1349          if (uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR) {
> > > > > > >   1350  #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> > > > > > >   1351                  goto out;
> > > > > > >   1352  #endif
> > > > > > >   1353                  vm_flags |= VM_UFFD_MINOR;
> > > > > > >   1354          }
> > > > > > >   1355
> > > > > > >   1356          ret = validate_range(mm, &uffdio_register.range.start,
> > > > > > >   1357                               uffdio_register.range.len);
> > > > > > >   1358          if (ret)
> > > > > > >   1359                  goto out;
> > > > > > >   1360
> > > > > > >   1361          start = uffdio_register.range.start;
> > > > > > >   1362          end = start + uffdio_register.range.len;
> > > > > > >   1363
> > > > > > >   1364          ret = -ENOMEM;
> > > > > > >   1365          if (!mmget_not_zero(mm))
> > > > > > >   1366                  goto out;
> > > > > > >   1367
> > > > > > >   1368          down_write(&mm->mmap_sem);
> > > > > > >   1369          if (!mmget_still_valid(mm))
> > > > > > >   1370                  goto out_unlock;
> > > > > > >   1371          vma = find_vma_prev(mm, start, &prev);
> > > > > > >   1372          if (!vma)
> > > > > > >   1373                  goto out_unlock;
> > > > > > >   1374
> > > > > > >   1375          /* check that there's at least one vma in the range */
> > > > > > >   1376          ret = -EINVAL;
> > > > > > >   1377          if (vma->vm_start >= end)
> > > > > > >   1378                  goto out_unlock;
> > > > > > >   1379
> > > > > > >   1380          /*
> > > > > > >   1381           * If the first vma contains huge pages, make sure start address
> > > > > > >   1382           * is aligned to huge page size.
> > > > > > >   1383           */
> > > > > > >   1384          if (is_vm_hugetlb_page(vma)) {
> > > > > > >   1385                  unsigned long vma_hpagesize = vma_kernel_pagesize(vma);
> > > > > > >   1386
> > > > > > >   1387                  if (start & (vma_hpagesize - 1))
> > > > > > >   1388                          goto out_unlock;
> > > > > > >   1389          }
> > > > > > >   1390
> > > > > > >   1391          /*
> > > > > > >   1392           * Search for not compatible vmas.
> > > > > > >   1393           */
> > > > > > >   1394          found = false;
> > > > > > >   1395          basic_ioctls = false;
> > > > > > >   1396          for (cur = vma; cur && cur->vm_start < end; cur = cur->vm_next) {
> > > > > > >   1397                  cond_resched();
> > > > > > >   1398
> > > > > > >   1399                  BUG_ON(!!cur->vm_userfaultfd_ctx.ctx ^
> > > > > > >   1400                         !!(cur->vm_flags & __VM_UFFD_FLAGS));
> > > > > > >   1401
> > > > > > >   1402                  /* check not compatible vmas */
> > > > > > >   1403                  ret = -EINVAL;
> > > > > > >   1404                  if (!vma_can_userfault(cur, vm_flags))
> > > > > > >   1405                          goto out_unlock;
> > > > > > >   1406
> > > > > > >   1407                  /*
> > > > > > >   1408                   * UFFDIO_COPY will fill file holes even without
> > > > > > >   1409                   * PROT_WRITE. This check enforces that if this is a
> > > > > > >   1410                   * MAP_SHARED, the process has write permission to the backing
> > > > > > >   1411                   * file. If VM_MAYWRITE is set it also enforces that on a
> > > > > > >   1412                   * MAP_SHARED vma: there is no F_WRITE_SEAL and no further
> > > > > > >   1413                   * F_WRITE_SEAL can be taken until the vma is destroyed.
> > > > > > >   1414                   */
> > > > > > >   1415                  ret = -EPERM;
> > > > > > >   1416                  if (unlikely(!(cur->vm_flags & VM_MAYWRITE)))
> > > > > > >   1417                          goto out_unlock;
> > > > > > >   1418
> > > > > > >   1419                  /*
> > > > > > >   1420                   * If this vma contains ending address, and huge pages
> > > > > > >   1421                   * check alignment.
> > > > > > >   1422                   */
> > > > > > >   1423                  if (is_vm_hugetlb_page(cur) && end <= cur->vm_end &&
> > > > > > >   1424                      end > cur->vm_start) {
> > > > > > >   1425                          unsigned long vma_hpagesize = vma_kernel_pagesize(cur);
> > > > > > >   1426
> > > > > > >   1427                          ret = -EINVAL;
> > > > > > >   1428
> > > > > > >   1429                          if (end & (vma_hpagesize - 1))
> > > > > > >   1430                                  goto out_unlock;
> > > > > > >   1431                  }
> > > > > > >   1432
> > > > > > >   1433                  /*
> > > > > > >   1434                   * Check that this vma isn't already owned by a
> > > > > > >   1435                   * different userfaultfd. We can't allow more than one
> > > > > > >   1436                   * userfaultfd to own a single vma simultaneously or we
> > > > > > >   1437                   * wouldn't know which one to deliver the userfaults to.
> > > > > > >   1438                   */
> > > > > > >   1439                  ret = -EBUSY;
> > > > > > >   1440                  if (cur->vm_userfaultfd_ctx.ctx &&
> > > > > > >   1441                      cur->vm_userfaultfd_ctx.ctx != ctx)
> > > > > > >   1442                          goto out_unlock;
> > > > > > >   1443
> > > > > > >   1444                  /*
> > > > > > >   1445                   * Note vmas containing huge pages
> > > > > > >   1446                   */
> > > > > > >   1447                  if (is_vm_hugetlb_page(cur))
> > > > > > >   1448                          basic_ioctls = true;
> > > > > > >   1449
> > > > > > >   1450                  found = true;
> > > > > > >   1451          }
> > > > > > >   1452          BUG_ON(!found);
> > > > > > >   1453
> > > > > > >   1454          if (vma->vm_start < start)
> > > > > > >   1455                  prev = vma;
> > > > > > >   1456
> > > > > > >   1457          ret = 0;
> > > > > > >   1458          do {
> > > > > > >   1459                  cond_resched();
> > > > > > >   1460
> > > > > > >   1461                  BUG_ON(!vma_can_userfault(vma, vm_flags));
> > > > > > >   1462                  BUG_ON(vma->vm_userfaultfd_ctx.ctx &&
> > > > > > >   1463                         vma->vm_userfaultfd_ctx.ctx != ctx);
> > > > > > >   1464                  WARN_ON(!(vma->vm_flags & VM_MAYWRITE));
> > > > > > >   1465
> > > > > > >   1466                  /*
> > > > > > >   1467                   * Nothing to do: this vma is already registered into this
> > > > > > >   1468                   * userfaultfd and with the right tracking mode too.
> > > > > > >   1469                   */
> > > > > > >   1470                  if (vma->vm_userfaultfd_ctx.ctx == ctx &&
> > > > > > >   1471                      (vma->vm_flags & vm_flags) == vm_flags)
> > > > > > >   1472                          goto skip;
> > > > > > >   1473
> > > > > > >   1474                  if (vma->vm_start > start)
> > > > > > >   1475                          start = vma->vm_start;
> > > > > > >   1476                  vma_end = min(end, vma->vm_end);
> > > > > > >   1477
> > > > > > >   1478                  new_flags = (vma->vm_flags & ~__VM_UFFD_FLAGS) | vm_flags;
> > > > > > >   1479                  prev = vma_merge(mm, prev, start, vma_end, new_flags,
> > > > > > >   1480                                   vma->anon_vma, vma->vm_file, vma->vm_pgoff,
> > > > > > >   1481                                   vma_policy(vma),
> > > > > > >   1482                                   ((struct vm_userfaultfd_ctx){ ctx }),
> > > > > > >   1483                                   vma_get_anon_name(vma));
> > > > > > >   1484                  if (prev) {
> > > > > > >   1485                          vma = prev;
> > > > > > >   1486                          goto next;
> > > > > > >   1487                  }
> > > > > > >   1488                  if (vma->vm_start < start) {
> > > > > > >   1489                          ret = split_vma(mm, vma, start, 1);
> > > > > > >   1490                          if (ret)
> > > > > > >   1491                                  break;
> > > > > > >   1492                  }
> > > > > > >   1493                  if (vma->vm_end > end) {
> > > > > > >   1494                          ret = split_vma(mm, vma, end, 0);
> > > > > > >   1495                          if (ret)
> > > > > > >   1496                                  break;
> > > > > > >   1497                  }
> > > > > > >   1498          next:
> > > > > > >   1499                  /*
> > > > > > >   1500                   * In the vma_merge() successful mprotect-like case 8:
> > > > > > >   1501                   * the next vma was merged into the current one and
> > > > > > >   1502                   * the current one has not been updated yet.
> > > > > > >   1503                   */
> > > > > > >   1504                  vma->vm_flags = new_flags;
> > > > > > >   1505                  vma->vm_userfaultfd_ctx.ctx = ctx;
> > > > > > >   1506
> > > > > > >   1507                  if (is_vm_hugetlb_page(vma) && uffd_disable_huge_pmd_share(vma))
> > > > > > >   1508                          hugetlb_unshare_all_pmds(vma);
> > > > > > >   1509
> > > > > > >   1510          skip:
> > > > > > >   1511                  prev = vma;
> > > > > > >   1512                  start = vma->vm_end;
> > > > > > >   1513                  vma = vma->vm_next;
> > > > > > >   1514          } while (vma && vma->vm_start < end);
> > > > > > >   1515  out_unlock:
> > > > > > >   1516          up_write(&mm->mmap_sem);
> > > > > > >   1517          mmput(mm);
> > > > > > >   1518          if (!ret) {
> > > > > > > > 1519                  __u64 ioctls_out;
> > > > > > >   1520
> > > > > > >   1521                  ioctls_out = basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > > > > >   1522                      UFFD_API_RANGE_IOCTLS;
> > > > > > >   1523
> > > > > > >   1524                  /* CONTINUE ioctl is only supported for MINOR ranges. */
> > > > > > >   1525                  if (!(uffdio_register.mode & UFFDIO_REGISTER_MODE_MINOR))
> > > > > > >   1526                          ioctls_out &= ~((__u64)1 << _UFFDIO_CONTINUE);
> > > > > > >   1527
> > > > > > >   1528                  /*
> > > > > > >   1529                   * Now that we scanned all vmas we can already tell
> > > > > > >   1530                   * userland which ioctls methods are guaranteed to
> > > > > > >   1531                   * succeed on this range.
> > > > > > >   1532                   */
> > > > > > >   1533                  if (put_user(basic_ioctls ? UFFD_API_RANGE_IOCTLS_BASIC :
> > > > > > >   1534                               UFFD_API_RANGE_IOCTLS,
> > > > > > >   1535                               &user_uffdio_register->ioctls))
> > > > > > >   1536                          ret = -EFAULT;
> > > > > > >   1537          }
> > > > > > >   1538  out:
> > > > > > >   1539          return ret;
> > > > > > >   1540  }
> > > > > > >   1541
> > > > > > >
> > > > > > > ---
> > > > > > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > > > > > https://lists.01.org/hyperkitty/list/[email protected]
> > > > > >
-- 
GWML mailing list
[email protected]
https://gwml.gnuweeb.org/listinfo/gwml

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-20 18:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-16 18:10 [ammarfaizi2-block:google/android/kernel/common/android12-5.4 6517/9999] fs/userfaultfd.c:1519:9: warning: variable 'ioctls_out' set but not used kernel test robot
2022-01-19 20:38 ` Axel Rasmussen
2022-01-19 21:19   ` Suren Baghdasaryan
2022-01-19 22:02     ` Axel Rasmussen
2022-01-19 22:10       ` Kalesh Singh
2022-01-19 22:11       ` Lokesh Gidra
2022-01-20 18:14         ` Axel Rasmussen
2022-01-20 18:16           ` Lokesh Gidra
2022-01-20 18:30             ` Axel Rasmussen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox