| Message ID | 20170805191924.5045-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Chris Wilson (2017-08-05 20:19:24) > The wait-ioctl is optionally supplied a timeout with nanosecond > precision in a s64 field. We use nsecs_to_jiffies64() to convert that > into the jiffies consumed by the scheduler, but internally > nsecs_to_jiffies64() does not guard against overflow (as it's purpose is > for use by the scheduler and not drivers!). So we must guard against the > overflow ourselves, and in the process note that we may then return > much earlier than the timeout selected by the user, so don't report > ETIME unless we do hit the timeout. (Woe betold us though if the user > waits for a year (32bit) and the request is still not complete!) > > Reported-by: Jason Ekstrand <jason.ekstrand@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_drv.h | 6 ++++++ > drivers/gpu/drm/i915/i915_gem.c | 4 ++++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2638967211a9..184f4d11de79 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -4144,6 +4144,12 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m) > > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) > { > +#if NSEC_PER_SEC % HZ > + /* nsecs_to_jiffies64() does not guard against overflow */ > + if (n >= (u64)MAX_JIFFY_OFFSET * NSEC_PER_SEC / HZ) > + return MAX_JIFFY_OFFSET; This still overflows, we need n / NSEC_PER_SEC >= MAX_JIFFY_OFFSET / HZ as MAX_JIFFY_OFFSET is ~LONG_MAX/2 Hmm, so div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ ?
On la, 2017-08-05 at 20:19 +0100, Chris Wilson wrote: > The wait-ioctl is optionally supplied a timeout with nanosecond > precision in a s64 field. We use nsecs_to_jiffies64() to convert that > into the jiffies consumed by the scheduler, but internally > nsecs_to_jiffies64() does not guard against overflow (as it's purpose is > for use by the scheduler and not drivers!). So we must guard against the > overflow ourselves, and in the process note that we may then return > much earlier than the timeout selected by the user, so don't report > ETIME unless we do hit the timeout. (Woe betold us though if the user > waits for a year (32bit) and the request is still not complete!) > > Reported-by: Jason Ekstrand <jason.ekstrand@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> <SNIP> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 44df7dc3f880..b5794add4a3a 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3543,6 +3543,10 @@ i915_gem_wait_ioctl(struct file *filp, > */ > if (ret == -ETIME && !nsecs_to_jiffies(arg.timeout_ns)) > arg.timeout_ns = 0; > + > + /* Asked to wait beyond the jiffie/scheduler precision */ > + if (ret == -ETIME && arg.timeout_ns) > + ret = -EAGAIN; -EAGAIN is documented as "GPU wedged" in the ioctl documentation. So better update that documentation. Regards, Joonas
On la, 2017-08-05 at 20:47 +0100, Chris Wilson wrote: > Quoting Chris Wilson (2017-08-05 20:19:24) <SNIP> > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -4144,6 +4144,12 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m) > > > > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) > > { > > +#if NSEC_PER_SEC % HZ > > + /* nsecs_to_jiffies64() does not guard against overflow */ > > + if (n >= (u64)MAX_JIFFY_OFFSET * NSEC_PER_SEC / HZ) > > + return MAX_JIFFY_OFFSET; > > This still overflows, we need n / NSEC_PER_SEC >= MAX_JIFFY_OFFSET / HZ > as MAX_JIFFY_OFFSET is ~LONG_MAX/2 > > Hmm, so div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ ? This reflects the original test best. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2638967211a9..184f4d11de79 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -4144,6 +4144,12 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m) static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) { +#if NSEC_PER_SEC % HZ + /* nsecs_to_jiffies64() does not guard against overflow */ + if (n >= (u64)MAX_JIFFY_OFFSET * NSEC_PER_SEC / HZ) + return MAX_JIFFY_OFFSET; +#endif + return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1); } diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 44df7dc3f880..b5794add4a3a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3543,6 +3543,10 @@ i915_gem_wait_ioctl(struct file *filp, */ if (ret == -ETIME && !nsecs_to_jiffies(arg.timeout_ns)) arg.timeout_ns = 0; + + /* Asked to wait beyond the jiffie/scheduler precision */ + if (ret == -ETIME && arg.timeout_ns) + ret = -EAGAIN; } i915_gem_object_put(obj);
The wait-ioctl is optionally supplied a timeout with nanosecond precision in a s64 field. We use nsecs_to_jiffies64() to convert that into the jiffies consumed by the scheduler, but internally nsecs_to_jiffies64() does not guard against overflow (as it's purpose is for use by the scheduler and not drivers!). So we must guard against the overflow ourselves, and in the process note that we may then return much earlier than the timeout selected by the user, so don't report ETIME unless we do hit the timeout. (Woe betold us though if the user waits for a year (32bit) and the request is still not complete!) Reported-by: Jason Ekstrand <jason.ekstrand@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_drv.h | 6 ++++++ drivers/gpu/drm/i915/i915_gem.c | 4 ++++ 2 files changed, 10 insertions(+)