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(-)
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)
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
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.
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
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 = 5294967295Does 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.
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 = 5294967295Does 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/d817f2c69615cf37b78f484a25b7...
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
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/d817f2c69615cf37b78f484a25b7...
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
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Guido Günther -
Lucas Stach -
Russell King - ARM Linux admin