| Message ID | 20190824135848.8571-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: Give dma-fence-array distinct lockclasses | expand |
Am 24.08.19 um 15:58 schrieb Chris Wilson: > In order to allow dma-fence-array as a generic container for fences, we > need to allow for it to contain other dma-fence-arrays. By giving each > dma-fence-array construction their own lockclass, we allow different > types of dma-fence-array to nest, but still do not allow on class of > dma-fence-array to contain itself (even though they have distinct > locks). > > In practice, this means that each subsystem gets its own dma-fence-array > class and we can freely use dma-fence-arrays as containers within the > dmabuf core without angering lockdep. I've considered this for as well. E.g. to use the dma_fence_array implementation instead of coming up with the dma_fence_chain container. But as it turned out when userspace can control nesting, it is trivial to chain enough dma_fence_arrays together to cause an in kernel stack overflow. Which in turn creates a really nice attack vector. So as long as userspace has control over dma_fence_array nesting this is a clear NAK and actually extremely dangerous. It actually took me quite a while to get the dma_fence_chain container recursion less to avoid stuff like this. Regards, Christian. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Christian König <christian.koenig@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/dma-buf/dma-fence-array.c | 13 ++++++++----- > include/linux/dma-fence-array.h | 16 ++++++++++++---- > 2 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index d3fbd950be94..d9bcdbb66d46 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -147,10 +147,11 @@ EXPORT_SYMBOL(dma_fence_array_ops); > * If @signal_on_any is true the fence array signals if any fence in the array > * signals, otherwise it signals when all fences in the array signal. > */ > -struct dma_fence_array *dma_fence_array_create(int num_fences, > - struct dma_fence **fences, > - u64 context, unsigned seqno, > - bool signal_on_any) > +struct dma_fence_array *__dma_fence_array_create(int num_fences, > + struct dma_fence **fences, > + u64 context, unsigned seqno, > + bool signal_on_any, > + struct lock_class_key *key) > { > struct dma_fence_array *array; > size_t size = sizeof(*array); > @@ -162,6 +163,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > return NULL; > > spin_lock_init(&array->lock); > + lockdep_set_class(&array->lock, key); > + > dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, > context, seqno); > init_irq_work(&array->work, irq_dma_fence_array_work); > @@ -174,7 +177,7 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > > return array; > } > -EXPORT_SYMBOL(dma_fence_array_create); > +EXPORT_SYMBOL(__dma_fence_array_create); > > /** > * dma_fence_match_context - Check if all fences are from the given context > diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h > index 303dd712220f..1395f9428cdb 100644 > --- a/include/linux/dma-fence-array.h > +++ b/include/linux/dma-fence-array.h > @@ -74,10 +74,18 @@ to_dma_fence_array(struct dma_fence *fence) > return container_of(fence, struct dma_fence_array, base); > } > > -struct dma_fence_array *dma_fence_array_create(int num_fences, > - struct dma_fence **fences, > - u64 context, unsigned seqno, > - bool signal_on_any); > +#define dma_fence_array_create(num, fences, context, seqno, any) ({ \ > + static struct lock_class_key __key; \ > + \ > + __dma_fence_array_create((num), (fences), (context), (seqno), (any), \ > + &__key); \ > +}) > + > +struct dma_fence_array *__dma_fence_array_create(int num_fences, > + struct dma_fence **fences, > + u64 context, unsigned seqno, > + bool signal_on_any, > + struct lock_class_key *key); > > bool dma_fence_match_context(struct dma_fence *fence, u64 context); >
Quoting Koenig, Christian (2019-08-24 20:04:43) > Am 24.08.19 um 15:58 schrieb Chris Wilson: > > In order to allow dma-fence-array as a generic container for fences, we > > need to allow for it to contain other dma-fence-arrays. By giving each > > dma-fence-array construction their own lockclass, we allow different > > types of dma-fence-array to nest, but still do not allow on class of > > dma-fence-array to contain itself (even though they have distinct > > locks). > > > > In practice, this means that each subsystem gets its own dma-fence-array > > class and we can freely use dma-fence-arrays as containers within the > > dmabuf core without angering lockdep. > > I've considered this for as well. E.g. to use the dma_fence_array > implementation instead of coming up with the dma_fence_chain container. > > But as it turned out when userspace can control nesting, it is trivial > to chain enough dma_fence_arrays together to cause an in kernel stack > overflow. Which in turn creates a really nice attack vector. > > So as long as userspace has control over dma_fence_array nesting this is > a clear NAK and actually extremely dangerous. You are proposing to use recursive dma_fence_array containers for dma_resv... > It actually took me quite a while to get the dma_fence_chain container > recursion less to avoid stuff like this. Sure, we've been avoiding recursion for years. -Chris
Am 24.08.19 um 21:12 schrieb Chris Wilson: > Quoting Koenig, Christian (2019-08-24 20:04:43) >> Am 24.08.19 um 15:58 schrieb Chris Wilson: >>> In order to allow dma-fence-array as a generic container for fences, we >>> need to allow for it to contain other dma-fence-arrays. By giving each >>> dma-fence-array construction their own lockclass, we allow different >>> types of dma-fence-array to nest, but still do not allow on class of >>> dma-fence-array to contain itself (even though they have distinct >>> locks). >>> >>> In practice, this means that each subsystem gets its own dma-fence-array >>> class and we can freely use dma-fence-arrays as containers within the >>> dmabuf core without angering lockdep. >> I've considered this for as well. E.g. to use the dma_fence_array >> implementation instead of coming up with the dma_fence_chain container. >> >> But as it turned out when userspace can control nesting, it is trivial >> to chain enough dma_fence_arrays together to cause an in kernel stack >> overflow. Which in turn creates a really nice attack vector. >> >> So as long as userspace has control over dma_fence_array nesting this is >> a clear NAK and actually extremely dangerous. > You are proposing to use recursive dma_fence_array containers for > dma_resv... Hui? Where? I've tried rather hard to avoid that. That was certainly not intentional, Christian. > >> It actually took me quite a while to get the dma_fence_chain container >> recursion less to avoid stuff like this. > Sure, we've been avoiding recursion for years. > -Chris
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index d3fbd950be94..d9bcdbb66d46 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -147,10 +147,11 @@ EXPORT_SYMBOL(dma_fence_array_ops); * If @signal_on_any is true the fence array signals if any fence in the array * signals, otherwise it signals when all fences in the array signal. */ -struct dma_fence_array *dma_fence_array_create(int num_fences, - struct dma_fence **fences, - u64 context, unsigned seqno, - bool signal_on_any) +struct dma_fence_array *__dma_fence_array_create(int num_fences, + struct dma_fence **fences, + u64 context, unsigned seqno, + bool signal_on_any, + struct lock_class_key *key) { struct dma_fence_array *array; size_t size = sizeof(*array); @@ -162,6 +163,8 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, return NULL; spin_lock_init(&array->lock); + lockdep_set_class(&array->lock, key); + dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, context, seqno); init_irq_work(&array->work, irq_dma_fence_array_work); @@ -174,7 +177,7 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, return array; } -EXPORT_SYMBOL(dma_fence_array_create); +EXPORT_SYMBOL(__dma_fence_array_create); /** * dma_fence_match_context - Check if all fences are from the given context diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h index 303dd712220f..1395f9428cdb 100644 --- a/include/linux/dma-fence-array.h +++ b/include/linux/dma-fence-array.h @@ -74,10 +74,18 @@ to_dma_fence_array(struct dma_fence *fence) return container_of(fence, struct dma_fence_array, base); } -struct dma_fence_array *dma_fence_array_create(int num_fences, - struct dma_fence **fences, - u64 context, unsigned seqno, - bool signal_on_any); +#define dma_fence_array_create(num, fences, context, seqno, any) ({ \ + static struct lock_class_key __key; \ + \ + __dma_fence_array_create((num), (fences), (context), (seqno), (any), \ + &__key); \ +}) + +struct dma_fence_array *__dma_fence_array_create(int num_fences, + struct dma_fence **fences, + u64 context, unsigned seqno, + bool signal_on_any, + struct lock_class_key *key); bool dma_fence_match_context(struct dma_fence *fence, u64 context);
In order to allow dma-fence-array as a generic container for fences, we need to allow for it to contain other dma-fence-arrays. By giving each dma-fence-array construction their own lockclass, we allow different types of dma-fence-array to nest, but still do not allow on class of dma-fence-array to contain itself (even though they have distinct locks). In practice, this means that each subsystem gets its own dma-fence-array class and we can freely use dma-fence-arrays as containers within the dmabuf core without angering lockdep. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Christian König <christian.koenig@amd.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/dma-buf/dma-fence-array.c | 13 ++++++++----- include/linux/dma-fence-array.h | 16 ++++++++++++---- 2 files changed, 20 insertions(+), 9 deletions(-)