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 <[email protected]>
Link: https://patchwork.kernel.org/patch/11291089/
Signed-off-by: Arnd Bergmann <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> Link: https://patchwork.kernel.org/patch/11291089/
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> 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.
--
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
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 <[email protected]>
>
> 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 <[email protected]>
> > Link: https://patchwork.kernel.org/patch/11291089/
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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
>
On Fri, Jan 24, 2020 at 9:56 AM Guido Günther <[email protected]> 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