| Message ID | 20200121114553.2667556-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | drm/etnaviv: only reject timeouts with tv_nsec >= 2 seconds | expand |
Hi, On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > sometimes passes timeouts with nanosecond values larger than 1000000000, > which gets rejected after my first patch. > > To avoid breaking this, while also not allowing completely arbitrary > values, set the limit to 1999999999 and use set_normalized_timespec64() > to get the correct format before comparing it. I'm seeing values up to 5 seconds so I need if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) to unbreak rendering. Which seems to match what mesa's get_abs_timeout() does and how it's invoked. with that: Tested-by: Guido Günther <agx@sigxcpu.org> Cheers, -- Guido > > This also addresses the off-by-1 glitch reported by Ben Hutchings. > > Fixes: 172a216ff334 ("drm/etnaviv: reject timeouts with tv_nsec >= NSEC_PER_SEC") > Cc: Guido Günther <agx@sigxcpu.org> > Link: https://patchwork.kernel.org/patch/11291089/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++--- > drivers/gpu/drm/etnaviv/etnaviv_drv.h | 6 ++---- > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > index 3eb0f9223bea..d94740c123d3 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > @@ -292,7 +292,11 @@ static int etnaviv_ioctl_gem_cpu_prep(struct drm_device *dev, void *data, > if (args->op & ~(ETNA_PREP_READ | ETNA_PREP_WRITE | ETNA_PREP_NOSYNC)) > return -EINVAL; > > - if (args->timeout.tv_nsec > NSEC_PER_SEC) > + /* > + * existing user space passes non-normalized timespecs, but never > + * more than 2 seconds worth of nanoseconds > + */ > + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) > return -EINVAL; > > obj = drm_gem_object_lookup(file, args->handle); > @@ -358,7 +362,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, > if (args->flags & ~(ETNA_WAIT_NONBLOCK)) > return -EINVAL; > > - if (args->timeout.tv_nsec > NSEC_PER_SEC) > + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) > return -EINVAL; > > if (args->pipe >= ETNA_MAX_PIPES) > @@ -412,7 +416,7 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, > if (args->flags & ~(ETNA_WAIT_NONBLOCK)) > return -EINVAL; > > - if (args->timeout.tv_nsec > NSEC_PER_SEC) > + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) > return -EINVAL; > > if (args->pipe >= ETNA_MAX_PIPES) > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > index efc656efeb0f..3e47050af706 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > @@ -109,12 +109,10 @@ static inline size_t size_vstruct(size_t nelem, size_t elem_size, size_t base) > static inline unsigned long etnaviv_timeout_to_jiffies( > const struct drm_etnaviv_timespec *timeout) > { > - struct timespec64 ts, to = { > - .tv_sec = timeout->tv_sec, > - .tv_nsec = timeout->tv_nsec, > - }; > + struct timespec64 ts, to; > > ktime_get_ts64(&ts); > + set_normalized_timespec64(&to, timeout->tv_sec, timeout->tv_nsec); > > /* timeouts before "now" have already expired */ > if (timespec64_compare(&to, &ts) <= 0) > -- > 2.25.0 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jan 21, 2020 at 01:55:46PM +0100, Guido Günther wrote: > Hi, > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > > sometimes passes timeouts with nanosecond values larger than 1000000000, > > which gets rejected after my first patch. > > > > To avoid breaking this, while also not allowing completely arbitrary > > values, set the limit to 1999999999 and use set_normalized_timespec64() > > to get the correct format before comparing it. > > I'm seeing values up to 5 seconds so I need > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) I assume you're looking at 64-bit, but I suspect userspace needs looking at considering 32-bit. If userspace uses a 32-bit tv_nsec anywhere in the path that it attempts to pass up to 5 seconds in tv_nsec, then this will fail to pass the correct timeout. If that is the case, userspace is buggy, and needs fixing not to pass such large values through tv_nsec irrespective of this issue. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Guido, On Di, 2020-01-21 at 13:55 +0100, Guido Günther wrote: > Hi, > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > > sometimes passes timeouts with nanosecond values larger than 1000000000, > > which gets rejected after my first patch. > > > > To avoid breaking this, while also not allowing completely arbitrary > > values, set the limit to 1999999999 and use set_normalized_timespec64() > > to get the correct format before comparing it. > > I'm seeing values up to 5 seconds so I need > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) > > to unbreak rendering. Which seems to match what mesa's get_abs_timeout() > does and how it's invoked. I have not tested this myself yet, only looked at the code. From the code I quoted earlier, I don't see how we end up with 5 * NSEC_PER_SEC in the tv_nsec member, even if the timeout passed to get_abs_timeout() is 5 seconds. Regards, Lucas > with that: > > Tested-by: Guido Günther <agx@sigxcpu.org> > > Cheers, > -- Guido > > > This also addresses the off-by-1 glitch reported by Ben Hutchings. > > > > Fixes: 172a216ff334 ("drm/etnaviv: reject timeouts with tv_nsec >= NSEC_PER_SEC") > > Cc: Guido Günther <agx@sigxcpu.org> > > Link: https://patchwork.kernel.org/patch/11291089/ > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++--- > > drivers/gpu/drm/etnaviv/etnaviv_drv.h | 6 ++---- > > 2 files changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > index 3eb0f9223bea..d94740c123d3 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > @@ -292,7 +292,11 @@ static int etnaviv_ioctl_gem_cpu_prep(struct drm_device *dev, void *data, > > if (args->op & ~(ETNA_PREP_READ | ETNA_PREP_WRITE | ETNA_PREP_NOSYNC)) > > return -EINVAL; > > > > - if (args->timeout.tv_nsec > NSEC_PER_SEC) > > + /* > > + * existing user space passes non-normalized timespecs, but never > > + * more than 2 seconds worth of nanoseconds > > + */ > > + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) > > return -EINVAL; > > > > obj = drm_gem_object_lookup(file, args->handle); > > @@ -358,7 +362,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, > > if (args->flags & ~(ETNA_WAIT_NONBLOCK)) > > return -EINVAL; > > > > - if (args->timeout.tv_nsec > NSEC_PER_SEC) > > + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) > > return -EINVAL; > > > > if (args->pipe >= ETNA_MAX_PIPES) > > @@ -412,7 +416,7 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, > > if (args->flags & ~(ETNA_WAIT_NONBLOCK)) > > return -EINVAL; > > > > - if (args->timeout.tv_nsec > NSEC_PER_SEC) > > + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) > > return -EINVAL; > > > > if (args->pipe >= ETNA_MAX_PIPES) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > > index efc656efeb0f..3e47050af706 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h > > @@ -109,12 +109,10 @@ static inline size_t size_vstruct(size_t nelem, size_t elem_size, size_t base) > > static inline unsigned long etnaviv_timeout_to_jiffies( > > const struct drm_etnaviv_timespec *timeout) > > { > > - struct timespec64 ts, to = { > > - .tv_sec = timeout->tv_sec, > > - .tv_nsec = timeout->tv_nsec, > > - }; > > + struct timespec64 ts, to; > > > > ktime_get_ts64(&ts); > > + set_normalized_timespec64(&to, timeout->tv_sec, timeout->tv_nsec); > > > > /* timeouts before "now" have already expired */ > > if (timespec64_compare(&to, &ts) <= 0) > > -- > > 2.25.0 > >
On Tue, Jan 21, 2020 at 5:10 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Guido, > > On Di, 2020-01-21 at 13:55 +0100, Guido Günther wrote: > > Hi, > > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > > > sometimes passes timeouts with nanosecond values larger than 1000000000, > > > which gets rejected after my first patch. > > > > > > To avoid breaking this, while also not allowing completely arbitrary > > > values, set the limit to 1999999999 and use set_normalized_timespec64() > > > to get the correct format before comparing it. > > > > I'm seeing values up to 5 seconds so I need > > > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) > > > > to unbreak rendering. Which seems to match what mesa's get_abs_timeout() > > does and how it's invoked. > > I have not tested this myself yet, only looked at the code. From the > code I quoted earlier, I don't see how we end up with 5 * NSEC_PER_SEC > in the tv_nsec member, even if the timeout passed to get_abs_timeout() > is 5 seconds. I can think of two different ways you'd end up with around five seconds here: a) you have a completely arbitrary 32-bit number through truncation, which is up to 4.2 seconds b) you have the same kind of 32-bit number, but add up to another 999999999 nanoseconds, so you get up to 5.2 seconds in the 64-bit field. It could of course be something completely different. If this works correctly today, we may need to allow any 64-bit input for the nanoseconds and do an expensive 64-bit div/mod in the kernel for normalization rather than the cheaper set_normalized_timespec64() from my patch. Arnd
Hi, On Tue, Jan 21, 2020 at 08:05:27PM +0100, Arnd Bergmann wrote: > On Tue, Jan 21, 2020 at 5:10 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Hi Guido, > > > > On Di, 2020-01-21 at 13:55 +0100, Guido Günther wrote: > > > Hi, > > > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > > > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > > > > sometimes passes timeouts with nanosecond values larger than 1000000000, > > > > which gets rejected after my first patch. > > > > > > > > To avoid breaking this, while also not allowing completely arbitrary > > > > values, set the limit to 1999999999 and use set_normalized_timespec64() > > > > to get the correct format before comparing it. > > > > > > I'm seeing values up to 5 seconds so I need > > > > > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) > > > > > > to unbreak rendering. Which seems to match what mesa's get_abs_timeout() > > > does and how it's invoked. > > > > I have not tested this myself yet, only looked at the code. From the > > code I quoted earlier, I don't see how we end up with 5 * NSEC_PER_SEC > > in the tv_nsec member, even if the timeout passed to get_abs_timeout() > > is 5 seconds. > > I can think of two different ways you'd end up with around five seconds here: > > a) you have a completely arbitrary 32-bit number through truncation, > which is up to 4.2 seconds > b) you have the same kind of 32-bit number, but add up to another 999999999 > nanoseconds, so you get up to 5.2 seconds in the 64-bit field. I've dumped out some values tv_nsec values with current mesa git on arm64: [ 33.699652] etnaviv_ioctl_gem_cpu_prep: 4990449401 [ 33.813081] etnaviv_ioctl_gem_cpu_prep: 5103872445 [ 33.822936] etnaviv_ioctl_gem_cpu_prep: 5113731286 [ 33.840963] etnaviv_ioctl_gem_cpu_prep: 5131762726 [ 33.854120] etnaviv_ioctl_gem_cpu_prep: 5144920127 [ 33.861426] etnaviv_ioctl_gem_cpu_prep: 5152227527 [ 33.872666] etnaviv_ioctl_gem_cpu_prep: 5163466968 [ 33.879485] etnaviv_ioctl_gem_cpu_prep: 5170286808 The problem is that in mesa/libdrm static inline void get_abs_timeout(struct drm_etnaviv_timespec *tv, uint64_t ns) { struct timespec t; uint32_t s = ns / 1000000000; clock_gettime(CLOCK_MONOTONIC, &t); tv->tv_sec = t.tv_sec + s; tv->tv_nsec = t.tv_nsec + ns - (s * 1000000000); ^^^^^^^^^^^^^^^ this overflows (since `s` is `uint_32t` and hence we substract a way too small value with ns = 5000000000 which mesa uses in etna_bo_cpu_prep. } So with current mesa/libdrm (which needs to be fixed) we'd have a maximum t.tv_nsec + ns - (s_max * 1000000000) 999999999 + 5000000000 - 705032704 = 5294967295 Does that make sense? If so that'd be the possible upper bound for the kernel. Note that this only applies to etnaviv_ioctl_gem_cpu_prep. While etnaviv_ioctl_wait_fence and etnaviv_ioctl_gem_wait are affected too i've not yet seen user space passing in larger values. Cheers, -- Guido > > It could of course be something completely different. If this works correctly > today, we may need to allow any 64-bit input for the nanoseconds and do > an expensive 64-bit div/mod in the kernel for normalization rather than the > cheaper set_normalized_timespec64() from my patch. > > Arnd > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jan 22, 2020 at 11:30:34AM +0100, Guido Günther wrote: > Hi, > On Tue, Jan 21, 2020 at 08:05:27PM +0100, Arnd Bergmann wrote: > > On Tue, Jan 21, 2020 at 5:10 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Hi Guido, > > > > > > On Di, 2020-01-21 at 13:55 +0100, Guido Günther wrote: > > > > Hi, > > > > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > > > > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > > > > > sometimes passes timeouts with nanosecond values larger than 1000000000, > > > > > which gets rejected after my first patch. > > > > > > > > > > To avoid breaking this, while also not allowing completely arbitrary > > > > > values, set the limit to 1999999999 and use set_normalized_timespec64() > > > > > to get the correct format before comparing it. > > > > > > > > I'm seeing values up to 5 seconds so I need > > > > > > > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) > > > > > > > > to unbreak rendering. Which seems to match what mesa's get_abs_timeout() > > > > does and how it's invoked. > > > > > > I have not tested this myself yet, only looked at the code. From the > > > code I quoted earlier, I don't see how we end up with 5 * NSEC_PER_SEC > > > in the tv_nsec member, even if the timeout passed to get_abs_timeout() > > > is 5 seconds. > > > > I can think of two different ways you'd end up with around five seconds here: > > > > a) you have a completely arbitrary 32-bit number through truncation, > > which is up to 4.2 seconds > > b) you have the same kind of 32-bit number, but add up to another 999999999 > > nanoseconds, so you get up to 5.2 seconds in the 64-bit field. > > I've dumped out some values tv_nsec values with current mesa git on arm64: > > [ 33.699652] etnaviv_ioctl_gem_cpu_prep: 4990449401 > [ 33.813081] etnaviv_ioctl_gem_cpu_prep: 5103872445 > [ 33.822936] etnaviv_ioctl_gem_cpu_prep: 5113731286 > [ 33.840963] etnaviv_ioctl_gem_cpu_prep: 5131762726 > [ 33.854120] etnaviv_ioctl_gem_cpu_prep: 5144920127 > [ 33.861426] etnaviv_ioctl_gem_cpu_prep: 5152227527 > [ 33.872666] etnaviv_ioctl_gem_cpu_prep: 5163466968 > [ 33.879485] etnaviv_ioctl_gem_cpu_prep: 5170286808 > > The problem is that in mesa/libdrm > > static inline void get_abs_timeout(struct drm_etnaviv_timespec *tv, uint64_t ns) > { > struct timespec t; > uint32_t s = ns / 1000000000; > clock_gettime(CLOCK_MONOTONIC, &t); > tv->tv_sec = t.tv_sec + s; > tv->tv_nsec = t.tv_nsec + ns - (s * 1000000000); > ^^^^^^^^^^^^^^^ > this overflows (since `s` is `uint_32t` and hence we substract a way > too small value with ns = 5000000000 which mesa uses in > etna_bo_cpu_prep. > } > > So with current mesa/libdrm (which needs to be fixed) we'd have a maximum > > t.tv_nsec + ns - (s_max * 1000000000) > > 999999999 + 5000000000 - 705032704 = 5294967295 > > Does that make sense? If so that'd be the possible upper bound for the > kernel. Note that this only applies to etnaviv_ioctl_gem_cpu_prep. While > etnaviv_ioctl_wait_fence and etnaviv_ioctl_gem_wait are affected too > i've not yet seen user space passing in larger values. Except the fact that the calculation being done above is buggy. Not only do we end up with tv_sec incremented by 5 seconds, but we also end up with tv_nsec containing around 5 seconds in nanoseconds, which means we end up with about a 10 second timeout. I think it would probably be better for the kernel to print a warning once when noticing over-large nsec values, suggesting a userspace upgrade is in order, but continue the existing behaviour. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Russel, On Wed, Jan 22, 2020 at 10:35:53AM +0000, Russell King - ARM Linux admin wrote: > On Wed, Jan 22, 2020 at 11:30:34AM +0100, Guido Günther wrote: > > Hi, > > On Tue, Jan 21, 2020 at 08:05:27PM +0100, Arnd Bergmann wrote: > > > On Tue, Jan 21, 2020 at 5:10 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Hi Guido, > > > > > > > > On Di, 2020-01-21 at 13:55 +0100, Guido Günther wrote: > > > > > Hi, > > > > > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > > > > > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > > > > > > sometimes passes timeouts with nanosecond values larger than 1000000000, > > > > > > which gets rejected after my first patch. > > > > > > > > > > > > To avoid breaking this, while also not allowing completely arbitrary > > > > > > values, set the limit to 1999999999 and use set_normalized_timespec64() > > > > > > to get the correct format before comparing it. > > > > > > > > > > I'm seeing values up to 5 seconds so I need > > > > > > > > > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) > > > > > > > > > > to unbreak rendering. Which seems to match what mesa's get_abs_timeout() > > > > > does and how it's invoked. > > > > > > > > I have not tested this myself yet, only looked at the code. From the > > > > code I quoted earlier, I don't see how we end up with 5 * NSEC_PER_SEC > > > > in the tv_nsec member, even if the timeout passed to get_abs_timeout() > > > > is 5 seconds. > > > > > > I can think of two different ways you'd end up with around five seconds here: > > > > > > a) you have a completely arbitrary 32-bit number through truncation, > > > which is up to 4.2 seconds > > > b) you have the same kind of 32-bit number, but add up to another 999999999 > > > nanoseconds, so you get up to 5.2 seconds in the 64-bit field. > > > > I've dumped out some values tv_nsec values with current mesa git on arm64: > > > > [ 33.699652] etnaviv_ioctl_gem_cpu_prep: 4990449401 > > [ 33.813081] etnaviv_ioctl_gem_cpu_prep: 5103872445 > > [ 33.822936] etnaviv_ioctl_gem_cpu_prep: 5113731286 > > [ 33.840963] etnaviv_ioctl_gem_cpu_prep: 5131762726 > > [ 33.854120] etnaviv_ioctl_gem_cpu_prep: 5144920127 > > [ 33.861426] etnaviv_ioctl_gem_cpu_prep: 5152227527 > > [ 33.872666] etnaviv_ioctl_gem_cpu_prep: 5163466968 > > [ 33.879485] etnaviv_ioctl_gem_cpu_prep: 5170286808 > > > > The problem is that in mesa/libdrm > > > > static inline void get_abs_timeout(struct drm_etnaviv_timespec *tv, uint64_t ns) > > { > > struct timespec t; > > uint32_t s = ns / 1000000000; > > clock_gettime(CLOCK_MONOTONIC, &t); > > tv->tv_sec = t.tv_sec + s; > > tv->tv_nsec = t.tv_nsec + ns - (s * 1000000000); > > ^^^^^^^^^^^^^^^ > > this overflows (since `s` is `uint_32t` and hence we substract a way > > too small value with ns = 5000000000 which mesa uses in > > etna_bo_cpu_prep. > > } > > > > So with current mesa/libdrm (which needs to be fixed) we'd have a maximum > > > > t.tv_nsec + ns - (s_max * 1000000000) > > > > 999999999 + 5000000000 - 705032704 = 5294967295 > > > > Does that make sense? If so that'd be the possible upper bound for the > > kernel. Note that this only applies to etnaviv_ioctl_gem_cpu_prep. While > > etnaviv_ioctl_wait_fence and etnaviv_ioctl_gem_wait are affected too > > i've not yet seen user space passing in larger values. > > Except the fact that the calculation being done above is buggy. > Not only do we end up with tv_sec incremented by 5 seconds, but > we also end up with tv_nsec containing around 5 seconds in > nanoseconds, which means we end up with about a 10 second timeout. yes. > > I think it would probably be better for the kernel to print a > warning once when noticing over-large nsec values, suggesting a > userspace upgrade is in order, but continue the existing behaviour. That makes sense to me. This also makes sure we don't break other (non mesa using) stuff accidentally. We have https://gitlab.freedesktop.org/mesa/mesa/commit/d817f2c69615cf37b78f484a25b7831ebe9dbe6f and https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3534 to normalize nsec to [0..999999999] now. Cheers, -- Guido > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jan 24, 2020 at 9:56 AM Guido Günther <agx@sigxcpu.org> wrote: > On Wed, Jan 22, 2020 at 10:35:53AM +0000, Russell King - ARM Linux admin wrote: > > On Wed, Jan 22, 2020 at 11:30:34AM +0100, Guido Günther wrote: > > I think it would probably be better for the kernel to print a > > warning once when noticing over-large nsec values, suggesting a > > userspace upgrade is in order, but continue the existing behaviour. > > That makes sense to me. This also makes sure we don't break other (non > mesa using) stuff accidentally. We have > > https://gitlab.freedesktop.org/mesa/mesa/commit/d817f2c69615cf37b78f484a25b7831ebe9dbe6f > > and > > https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3534 > > to normalize nsec to [0..999999999] now. > I have reverted my patch that adds the range check now, so I can send the rest of the series for inclusion. I've played around with ways to add a ratelimited warning message and to make sure that a malicious application cannot cause a long stall, but haven't managed to get a version I'm actually happy with. I'll follow up once the series is merged, and then we can add a better workaround later through the drm tree. Arnd
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c index 3eb0f9223bea..d94740c123d3 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c @@ -292,7 +292,11 @@ static int etnaviv_ioctl_gem_cpu_prep(struct drm_device *dev, void *data, if (args->op & ~(ETNA_PREP_READ | ETNA_PREP_WRITE | ETNA_PREP_NOSYNC)) return -EINVAL; - if (args->timeout.tv_nsec > NSEC_PER_SEC) + /* + * existing user space passes non-normalized timespecs, but never + * more than 2 seconds worth of nanoseconds + */ + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) return -EINVAL; obj = drm_gem_object_lookup(file, args->handle); @@ -358,7 +362,7 @@ static int etnaviv_ioctl_wait_fence(struct drm_device *dev, void *data, if (args->flags & ~(ETNA_WAIT_NONBLOCK)) return -EINVAL; - if (args->timeout.tv_nsec > NSEC_PER_SEC) + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) return -EINVAL; if (args->pipe >= ETNA_MAX_PIPES) @@ -412,7 +416,7 @@ static int etnaviv_ioctl_gem_wait(struct drm_device *dev, void *data, if (args->flags & ~(ETNA_WAIT_NONBLOCK)) return -EINVAL; - if (args->timeout.tv_nsec > NSEC_PER_SEC) + if (args->timeout.tv_nsec >= (2 * NSEC_PER_SEC)) return -EINVAL; if (args->pipe >= ETNA_MAX_PIPES) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h index efc656efeb0f..3e47050af706 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h @@ -109,12 +109,10 @@ static inline size_t size_vstruct(size_t nelem, size_t elem_size, size_t base) static inline unsigned long etnaviv_timeout_to_jiffies( const struct drm_etnaviv_timespec *timeout) { - struct timespec64 ts, to = { - .tv_sec = timeout->tv_sec, - .tv_nsec = timeout->tv_nsec, - }; + struct timespec64 ts, to; ktime_get_ts64(&ts); + set_normalized_timespec64(&to, timeout->tv_sec, timeout->tv_nsec); /* timeouts before "now" have already expired */ if (timespec64_compare(&to, &ts) <= 0)
As Guido Günther reported, get_abs_timeout() in the etnaviv user space sometimes passes timeouts with nanosecond values larger than 1000000000, which gets rejected after my first patch. To avoid breaking this, while also not allowing completely arbitrary values, set the limit to 1999999999 and use set_normalized_timespec64() to get the correct format before comparing it. This also addresses the off-by-1 glitch reported by Ben Hutchings. Fixes: 172a216ff334 ("drm/etnaviv: reject timeouts with tv_nsec >= NSEC_PER_SEC") Cc: Guido Günther <agx@sigxcpu.org> Link: https://patchwork.kernel.org/patch/11291089/ Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++--- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 6 ++---- 2 files changed, 9 insertions(+), 7 deletions(-)