Re: [Intel-gfx] [PATCH] drm/i915/gem: Promote 'remain' to unsigned long

[kbuild test robot <lkp@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5579 bytes --]

Hi Chris,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on next-20200406]
[cannot apply to drm-intel/for-linux-next v5.6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-gem-Promote-remain-to-unsigned-long/20200407-185221
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/iova.h:13:0,
                    from include/linux/intel-iommu.h:14,
                    from drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:7:
   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: In function 'eb_relocate_vma':
   include/linux/kernel.h:835:29: warning: comparison of distinct pointer types lacks a cast
      (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                ^
   include/linux/kernel.h:849:4: note: in expansion of macro '__typecheck'
      (__typecheck(x, y) && __no_side_effects(x, y))
       ^~~~~~~~~~~
   include/linux/kernel.h:859:24: note: in expansion of macro '__safe_cmp'
     __builtin_choose_expr(__safe_cmp(x, y), \
                           ^~~~~~~~~~
   include/linux/kernel.h:868:19: note: in expansion of macro '__careful_cmp'
    #define min(x, y) __careful_cmp(x, y, <)
                      ^~~~~~~~~~~~~
>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1523:24: note: in expansion of macro 'min'
      unsigned int count = min(remain, ARRAY_SIZE(stack));
                           ^~~

vim +/min +1523 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c

  1499	
  1500	static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
  1501	{
  1502	#define N_RELOC(x) ((x) / sizeof(struct drm_i915_gem_relocation_entry))
  1503		struct drm_i915_gem_relocation_entry stack[N_RELOC(512)];
  1504		struct drm_i915_gem_relocation_entry __user *urelocs;
  1505		const struct drm_i915_gem_exec_object2 *entry = ev->exec;
  1506		unsigned long remain;
  1507	
  1508		urelocs = u64_to_user_ptr(entry->relocs_ptr);
  1509		remain = entry->relocation_count;
  1510		if (unlikely(remain > N_RELOC(ULONG_MAX)))
  1511			return -EINVAL;
  1512	
  1513		/*
  1514		 * We must check that the entire relocation array is safe
  1515		 * to read. However, if the array is not writable the user loses
  1516		 * the updated relocation values.
  1517		 */
  1518		if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs))))
  1519			return -EFAULT;
  1520	
  1521		do {
  1522			struct drm_i915_gem_relocation_entry *r = stack;
> 1523			unsigned int count = min(remain, ARRAY_SIZE(stack));
  1524			unsigned int copied;
  1525	
  1526			/*
  1527			 * This is the fast path and we cannot handle a pagefault
  1528			 * whilst holding the struct mutex lest the user pass in the
  1529			 * relocations contained within a mmaped bo. For in such a case
  1530			 * we, the page fault handler would call i915_gem_fault() and
  1531			 * we would try to acquire the struct mutex again. Obviously
  1532			 * this is bad and so lockdep complains vehemently.
  1533			 */
  1534			copied = __copy_from_user(r, urelocs, count * sizeof(r[0]));
  1535			if (unlikely(copied)) {
  1536				remain = -EFAULT;
  1537				goto out;
  1538			}
  1539	
  1540			remain -= count;
  1541			do {
  1542				u64 offset = eb_relocate_entry(eb, ev, r);
  1543	
  1544				if (likely(offset == 0)) {
  1545				} else if ((s64)offset < 0) {
  1546					remain = (int)offset;
  1547					goto out;
  1548				} else {
  1549					/*
  1550					 * Note that reporting an error now
  1551					 * leaves everything in an inconsistent
  1552					 * state as we have *already* changed
  1553					 * the relocation value inside the
  1554					 * object. As we have not changed the
  1555					 * reloc.presumed_offset or will not
  1556					 * change the execobject.offset, on the
  1557					 * call we may not rewrite the value
  1558					 * inside the object, leaving it
  1559					 * dangling and causing a GPU hang. Unless
  1560					 * userspace dynamically rebuilds the
  1561					 * relocations on each execbuf rather than
  1562					 * presume a static tree.
  1563					 *
  1564					 * We did previously check if the relocations
  1565					 * were writable (access_ok), an error now
  1566					 * would be a strange race with mprotect,
  1567					 * having already demonstrated that we
  1568					 * can read from this userspace address.
  1569					 */
  1570					offset = gen8_canonical_addr(offset & ~UPDATE);
  1571					__put_user(offset,
  1572						   &urelocs[r - stack].presumed_offset);
  1573				}
  1574			} while (r++, --count);
  1575			urelocs += ARRAY_SIZE(stack);
  1576		} while (remain);
  1577	out:
  1578		reloc_cache_reset(&eb->reloc_cache);
  1579		return remain;
  1580	}
  1581	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 71902 bytes --]