Re: [PATCH] dma-buf: Give dma-fence-array distinct lockclasses
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dma-buf: Give dma-fence-array distinct lockclasses
Hi Chris, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [cannot apply to v5.3-rc5 next-20190823] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Chris-Wilson/dma-buf-Give-dma-fence-array-distinct-lockclasses/20190825-045722 reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot All warnings (new ones prefixed by >>): Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme. WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) include/linux/w1.h:272: warning: Function parameter or member 'of_match_table' not described in 'w1_family' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quotactl' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quota_on' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'getprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'setprocattr' not described in 'security_list_options' lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found lib/genalloc.c:1: warning: 'gen_pool_alloc' not found lib/genalloc.c:1: warning: 'gen_pool_free' not found lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client' include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device' drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found fs/direct-io.c:258: warning: Excess function parameter 'offset' description in 'dio_complete' fs/libfs.c:496: warning: Excess function parameter 'available' description in 'simple_write_end' fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode' include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops' include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry' include/linux/skbuff.h:893: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'list' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'head_frag' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or member 'encapsulation' not described in 'sk_buff' include/linux/skbuff.h:893: warning: Function parameter or
Re: [PATCH] dma-buf: Give dma-fence-array distinct lockclasses
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dma-buf: Give dma-fence-array distinct lockclasses
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
> Cc: Christian König
> Cc: Daniel Vetter
> ---
> 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(>lock);
> + lockdep_set_class(>lock, key);
> +
> dma_fence_init(>base, _fence_array_ops, >lock,
> context, seqno);
> init_irq_work(>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);
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] dma-buf: Give dma-fence-array distinct lockclasses
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
Cc: Christian König
Cc: Daniel Vetter
---
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(>lock);
+ lockdep_set_class(>lock, key);
+
dma_fence_init(>base, _fence_array_ops, >lock,
context, seqno);
init_irq_work(>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);
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
