| Message ID | 20230321102855.346732-4-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DELETE_BUF ioctl | expand |
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.3-rc3 next-20230321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230321102855.346732-4-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20230322/202303220057.J83sWVI1-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/aab64e29070dfec3a043b5020399f79554d6cae4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
git checkout aab64e29070dfec3a043b5020399f79554d6cae4
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303220057.J83sWVI1-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/module.h:22,
from drivers/media/common/videobuf2/videobuf2-core.c:21:
drivers/media/common/videobuf2/videobuf2-core.c: In function '__check_max_vb_buffer_per_queue':
>> include/linux/moduleparam.h:150:34: error: returning 'size_t *' {aka 'unsigned int *'} from a function with incompatible return type 'long unsigned int *' [-Werror=incompatible-pointer-types]
150 | param_check_##type(name, &(value)); \
| ^
include/linux/moduleparam.h:409:75: note: in definition of macro '__param_check'
409 | static inline type __always_unused *__check_##name(void) { return(p); }
| ^
include/linux/moduleparam.h:150:9: note: in expansion of macro 'param_check_ulong'
150 | param_check_##type(name, &(value)); \
| ^~~~~~~~~~~~
include/linux/moduleparam.h:127:9: note: in expansion of macro 'module_param_named'
127 | module_param_named(name, name, type, perm)
| ^~~~~~~~~~~~~~~~~~
drivers/media/common/videobuf2/videobuf2-core.c:37:1: note: in expansion of macro 'module_param'
37 | module_param(max_vb_buffer_per_queue, ulong, 0644);
| ^~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +150 include/linux/moduleparam.h
^1da177e4c3f415 Linus Torvalds 2005-04-16 100
546970bc6afc7fb Rusty Russell 2010-08-11 101 /**
546970bc6afc7fb Rusty Russell 2010-08-11 102 * module_param - typesafe helper for a module/cmdline parameter
e2854a1054ab171 Zhenzhong Duan 2019-11-04 103 * @name: the variable to alter, and exposed parameter name.
546970bc6afc7fb Rusty Russell 2010-08-11 104 * @type: the type of the parameter
546970bc6afc7fb Rusty Russell 2010-08-11 105 * @perm: visibility in sysfs.
546970bc6afc7fb Rusty Russell 2010-08-11 106 *
e2854a1054ab171 Zhenzhong Duan 2019-11-04 107 * @name becomes the module parameter, or (prefixed by KBUILD_MODNAME and a
546970bc6afc7fb Rusty Russell 2010-08-11 108 * ".") the kernel commandline parameter. Note that - is changed to _, so
546970bc6afc7fb Rusty Russell 2010-08-11 109 * the user can use "foo-bar=1" even for variable "foo_bar".
546970bc6afc7fb Rusty Russell 2010-08-11 110 *
c6a8b84da4c28bd Randy Dunlap 2020-07-17 111 * @perm is 0 if the variable is not to appear in sysfs, or 0444
546970bc6afc7fb Rusty Russell 2010-08-11 112 * for world-readable, 0644 for root-writable, etc. Note that if it
b51d23e4e9fea6f Dan Streetman 2015-06-17 113 * is writable, you may need to use kernel_param_lock() around
546970bc6afc7fb Rusty Russell 2010-08-11 114 * accesses (esp. charp, which can be kfreed when it changes).
546970bc6afc7fb Rusty Russell 2010-08-11 115 *
546970bc6afc7fb Rusty Russell 2010-08-11 116 * The @type is simply pasted to refer to a param_ops_##type and a
546970bc6afc7fb Rusty Russell 2010-08-11 117 * param_check_##type: for convenience many standard types are provided but
546970bc6afc7fb Rusty Russell 2010-08-11 118 * you can create your own by defining those variables.
546970bc6afc7fb Rusty Russell 2010-08-11 119 *
546970bc6afc7fb Rusty Russell 2010-08-11 120 * Standard types are:
7d8365771ffb0ed Paul Menzel 2020-07-03 121 * byte, hexint, short, ushort, int, uint, long, ulong
546970bc6afc7fb Rusty Russell 2010-08-11 122 * charp: a character pointer
546970bc6afc7fb Rusty Russell 2010-08-11 123 * bool: a bool, values 0/1, y/n, Y/N.
546970bc6afc7fb Rusty Russell 2010-08-11 124 * invbool: the above, only sense-reversed (N = true).
546970bc6afc7fb Rusty Russell 2010-08-11 125 */
546970bc6afc7fb Rusty Russell 2010-08-11 126 #define module_param(name, type, perm) \
546970bc6afc7fb Rusty Russell 2010-08-11 127 module_param_named(name, name, type, perm)
546970bc6afc7fb Rusty Russell 2010-08-11 128
3baee201b06cfaf Jani Nikula 2014-08-27 129 /**
3baee201b06cfaf Jani Nikula 2014-08-27 130 * module_param_unsafe - same as module_param but taints kernel
b6d0531ec7e2ae9 Fabien Dessenne 2019-12-02 131 * @name: the variable to alter, and exposed parameter name.
b6d0531ec7e2ae9 Fabien Dessenne 2019-12-02 132 * @type: the type of the parameter
b6d0531ec7e2ae9 Fabien Dessenne 2019-12-02 133 * @perm: visibility in sysfs.
3baee201b06cfaf Jani Nikula 2014-08-27 134 */
3baee201b06cfaf Jani Nikula 2014-08-27 135 #define module_param_unsafe(name, type, perm) \
3baee201b06cfaf Jani Nikula 2014-08-27 136 module_param_named_unsafe(name, name, type, perm)
3baee201b06cfaf Jani Nikula 2014-08-27 137
546970bc6afc7fb Rusty Russell 2010-08-11 138 /**
546970bc6afc7fb Rusty Russell 2010-08-11 139 * module_param_named - typesafe helper for a renamed module/cmdline parameter
546970bc6afc7fb Rusty Russell 2010-08-11 140 * @name: a valid C identifier which is the parameter name.
546970bc6afc7fb Rusty Russell 2010-08-11 141 * @value: the actual lvalue to alter.
546970bc6afc7fb Rusty Russell 2010-08-11 142 * @type: the type of the parameter
546970bc6afc7fb Rusty Russell 2010-08-11 143 * @perm: visibility in sysfs.
546970bc6afc7fb Rusty Russell 2010-08-11 144 *
546970bc6afc7fb Rusty Russell 2010-08-11 145 * Usually it's a good idea to have variable names and user-exposed names the
546970bc6afc7fb Rusty Russell 2010-08-11 146 * same, but that's harder if the variable must be non-static or is inside a
546970bc6afc7fb Rusty Russell 2010-08-11 147 * structure. This allows exposure under a different name.
546970bc6afc7fb Rusty Russell 2010-08-11 148 */
546970bc6afc7fb Rusty Russell 2010-08-11 149 #define module_param_named(name, value, type, perm) \
546970bc6afc7fb Rusty Russell 2010-08-11 @150 param_check_##type(name, &(value)); \
546970bc6afc7fb Rusty Russell 2010-08-11 151 module_param_cb(name, ¶m_ops_##type, &value, perm); \
546970bc6afc7fb Rusty Russell 2010-08-11 152 __MODULE_PARM_TYPE(name, #type)
546970bc6afc7fb Rusty Russell 2010-08-11 153
Hi Benjamin,
I love your patch! Yet something to improve:
[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.3-rc3 next-20230321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
base: git://linuxtv.org/media_tree.git master
patch link: https://lore.kernel.org/r/20230321102855.346732-4-benjamin.gaignard%40collabora.com
patch subject: [PATCH v2 3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage
config: hexagon-randconfig-r015-20230319 (https://download.01.org/0day-ci/archive/20230322/202303220301.fby6vJcX-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/aab64e29070dfec3a043b5020399f79554d6cae4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Gaignard/media-videobuf2-Access-vb2_queue-bufs-array-through-helper-functions/20230321-183154
git checkout aab64e29070dfec3a043b5020399f79554d6cae4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/media/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303220301.fby6vJcX-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/media/common/videobuf2/videobuf2-core.c:29:
In file included from include/media/videobuf2-core.h:19:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/media/common/videobuf2/videobuf2-core.c:29:
In file included from include/media/videobuf2-core.h:19:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/media/common/videobuf2/videobuf2-core.c:29:
In file included from include/media/videobuf2-core.h:19:
In file included from include/linux/dma-buf.h:16:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/media/common/videobuf2/videobuf2-core.c:37:1: error: incompatible pointer types returning 'size_t *' (aka 'unsigned int *') from a function with result type 'unsigned long *' [-Werror,-Wincompatible-pointer-types]
module_param(max_vb_buffer_per_queue, ulong, 0644);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/moduleparam.h:127:2: note: expanded from macro 'module_param'
module_param_named(name, name, type, perm)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/moduleparam.h:150:2: note: expanded from macro 'module_param_named'
param_check_##type(name, &(value)); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<scratch space>:65:1: note: expanded from here
param_check_ulong
^
include/linux/moduleparam.h:446:36: note: expanded from macro 'param_check_ulong'
#define param_check_ulong(name, p) __param_check(name, p, unsigned long)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/moduleparam.h:409:67: note: expanded from macro '__param_check'
static inline type __always_unused *__check_##name(void) { return(p); }
^~~
6 warnings and 1 error generated.
vim +37 drivers/media/common/videobuf2/videobuf2-core.c
36
> 37 module_param(max_vb_buffer_per_queue, ulong, 0644);
38
Hi Benjamin, >Add module parameter "max_vb_buffer_per_queue" to be able to limit the >number of vb2 buffers store in queue. > >Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >--- > drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > include/media/videobuf2-core.h | 11 +++++++++-- > 2 files changed, 12 insertions(+), 14 deletions(-) > >diff --git a/drivers/media/common/videobuf2/videobuf2-core.c >b/drivers/media/common/videobuf2/videobuf2-core.c >index ae9d72f4d181..f4da917ccf3f 100644 >--- a/drivers/media/common/videobuf2/videobuf2-core.c >+++ b/drivers/media/common/videobuf2/videobuf2-core.c >@@ -34,6 +34,8 @@ > static int debug; > module_param(debug, int, 0644); > >+module_param(max_vb_buffer_per_queue, ulong, 0644); >+ > #define dprintk(q, level, fmt, arg...) \ > do { \ > if (debug >= level) \ >@@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, >enum vb2_memory memory, > struct vb2_buffer *vb; > int ret; > >- /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME >*/ >- num_buffers = min_t(unsigned int, num_buffers, >- VB2_MAX_FRAME - q->num_buffers); >- > for (buffer = 0; buffer < num_buffers; ++buffer) { > /* Allocate vb2 buffer structures */ > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); @@ -801,9 +799,7 >@@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ >- WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); >- num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > * Set this now to ensure that drivers see the correct q->memory value >@@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, >enum vb2_memory memory, > bool no_previous_buffers = !q->num_buffers; > int ret; > >- if (q->num_buffers == VB2_MAX_FRAME) { >- dprintk(q, 1, "maximum number of buffers already allocated\n"); >- return -ENOBUFS; >- } >- > if (no_previous_buffers) { > if (q->waiting_in_dqbuf && *count) { > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); >@@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum >vb2_memory memory, > return -EINVAL; > } > >- num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); >+ num_buffers = *count; > > if (requested_planes && requested_sizes) { > num_planes = requested_planes; diff --git >a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index >397dbf6e61e1..b8b34a993e04 100644 >--- a/include/media/videobuf2-core.h >+++ b/include/media/videobuf2-core.h >@@ -12,6 +12,7 @@ > #ifndef _MEDIA_VIDEOBUF2_CORE_H > #define _MEDIA_VIDEOBUF2_CORE_H > >+#include <linux/minmax.h> > #include <linux/mm_types.h> > #include <linux/mutex.h> > #include <linux/poll.h> >@@ -48,6 +49,8 @@ struct vb2_fileio_data; struct vb2_threadio_data; struct >vb2_buffer; > >+static size_t max_vb_buffer_per_queue = 1024; >+ > /** > * struct vb2_mem_ops - memory handling/memory allocator operations. > * @alloc: allocate video memory and, optionally, allocator private data, >@@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct >vb2_queue *q, struct vb2_buffer * > > if (vb->index >= q->max_num_bufs) { > struct vb2_buffer **tmp; >+ int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * >+ 2); >+ >+ if (cnt >= q->max_num_bufs) >+ goto realloc_failed; > Is it likely that goto realloc_failed directly? The cnt is likely equal to q->max_num_bufs * 2, and it will greater than q->max_num_bufs. For example, the default value of q->max_num_bufs is 32, when vb->index comes to 32, cnt is 64, it's greater than 32, then goto recalloc_failed? >- tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q- >>bufs), GFP_KERNEL); >+ tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), >+ GFP_KERNEL); > if (!tmp) > goto realloc_failed; > >- q->max_num_bufs *= 2; >+ q->max_num_bufs = cnt; > q->bufs = tmp; > } > >-- >2.34.1
On Tue, Mar 21, 2023 at 11:28:50AM +0100, Benjamin Gaignard wrote: > Add module parameter "max_vb_buffer_per_queue" to be able to limit > the number of vb2 buffers store in queue. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > include/media/videobuf2-core.h | 11 +++++++++-- > 2 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index ae9d72f4d181..f4da917ccf3f 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -34,6 +34,8 @@ > static int debug; > module_param(debug, int, 0644); > > +module_param(max_vb_buffer_per_queue, ulong, 0644); > + > #define dprintk(q, level, fmt, arg...) \ > do { \ > if (debug >= level) \ > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > struct vb2_buffer *vb; > int ret; > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > - num_buffers = min_t(unsigned int, num_buffers, > - VB2_MAX_FRAME - q->num_buffers); > - We should keep the validation here, just using the new module parameter instead of VB2_MAX_FRAME. Otherwise we let the userspace pass UINT_MAX to REQBUFS and have the array below exhaust the system memory. > for (buffer = 0; buffer < num_buffers; ++buffer) { > /* Allocate vb2 buffer structures */ > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); Similar concern here. > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > * Set this now to ensure that drivers see the correct q->memory value > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > bool no_previous_buffers = !q->num_buffers; > int ret; > > - if (q->num_buffers == VB2_MAX_FRAME) { > - dprintk(q, 1, "maximum number of buffers already allocated\n"); > - return -ENOBUFS; > - } > - Ditto. > if (no_previous_buffers) { > if (q->waiting_in_dqbuf && *count) { > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > return -EINVAL; > } > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > + num_buffers = *count; Ditto. > > if (requested_planes && requested_sizes) { > num_planes = requested_planes; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 397dbf6e61e1..b8b34a993e04 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -12,6 +12,7 @@ > #ifndef _MEDIA_VIDEOBUF2_CORE_H > #define _MEDIA_VIDEOBUF2_CORE_H > > +#include <linux/minmax.h> > #include <linux/mm_types.h> > #include <linux/mutex.h> > #include <linux/poll.h> > @@ -48,6 +49,8 @@ struct vb2_fileio_data; > struct vb2_threadio_data; > struct vb2_buffer; > > +static size_t max_vb_buffer_per_queue = 1024; > + > /** > * struct vb2_mem_ops - memory handling/memory allocator operations. > * @alloc: allocate video memory and, optionally, allocator private data, > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * > > if (vb->index >= q->max_num_bufs) { > struct vb2_buffer **tmp; > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); Should cnt also be size_t to match max_vb_buffer_per_queue? This could also overflow given q->max_num_bufs big enough, so maybe it would just be better to rewrite to? size_t cnt = (q->max_num_bufs > max_vb_buffer_per_queue / 2) ? max_vb_buffer_per_queue : q->max_num_bufs * 2; Or we could just switch to XArray and it would solve this for us. :) Best regards, Tomasz > + > + if (cnt >= q->max_num_bufs) > + goto realloc_failed; > > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); > if (!tmp) > goto realloc_failed; > > - q->max_num_bufs *= 2; > + q->max_num_bufs = cnt; > q->bufs = tmp; > } > > -- > 2.34.1 >
On Fri, May 19, 2023 at 10:19:16AM +0000, Tomasz Figa wrote: > On Tue, Mar 21, 2023 at 11:28:50AM +0100, Benjamin Gaignard wrote: > > Add module parameter "max_vb_buffer_per_queue" to be able to limit > > the number of vb2 buffers store in queue. > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > --- > > drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > > include/media/videobuf2-core.h | 11 +++++++++-- > > 2 files changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index ae9d72f4d181..f4da917ccf3f 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -34,6 +34,8 @@ > > static int debug; > > module_param(debug, int, 0644); > > > > +module_param(max_vb_buffer_per_queue, ulong, 0644); > > + > > #define dprintk(q, level, fmt, arg...) \ > > do { \ > > if (debug >= level) \ > > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > > struct vb2_buffer *vb; > > int ret; > > > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > > - num_buffers = min_t(unsigned int, num_buffers, > > - VB2_MAX_FRAME - q->num_buffers); > > - > > We should keep the validation here, just using the new module parameter > instead of VB2_MAX_FRAME. Otherwise we let the userspace pass > UINT_MAX to REQBUFS and have the array below exhaust the system memory. > > > for (buffer = 0; buffer < num_buffers; ++buffer) { > > /* Allocate vb2 buffer structures */ > > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); > > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > /* > > * Make sure the requested values and current defaults are sane. > > */ > > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > > Similar concern here. > > > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > > /* > > * Set this now to ensure that drivers see the correct q->memory value > > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > bool no_previous_buffers = !q->num_buffers; > > int ret; > > > > - if (q->num_buffers == VB2_MAX_FRAME) { > > - dprintk(q, 1, "maximum number of buffers already allocated\n"); > > - return -ENOBUFS; > > - } > > - > > Ditto. > > > if (no_previous_buffers) { > > if (q->waiting_in_dqbuf && *count) { > > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); > > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > return -EINVAL; > > } > > > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > > + num_buffers = *count; > > Ditto. > > > > > if (requested_planes && requested_sizes) { > > num_planes = requested_planes; > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > > index 397dbf6e61e1..b8b34a993e04 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -12,6 +12,7 @@ > > #ifndef _MEDIA_VIDEOBUF2_CORE_H > > #define _MEDIA_VIDEOBUF2_CORE_H > > > > +#include <linux/minmax.h> > > #include <linux/mm_types.h> > > #include <linux/mutex.h> > > #include <linux/poll.h> > > @@ -48,6 +49,8 @@ struct vb2_fileio_data; > > struct vb2_threadio_data; > > struct vb2_buffer; > > > > +static size_t max_vb_buffer_per_queue = 1024; > > + > > /** > > * struct vb2_mem_ops - memory handling/memory allocator operations. > > * @alloc: allocate video memory and, optionally, allocator private data, > > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * > > > > if (vb->index >= q->max_num_bufs) { > > struct vb2_buffer **tmp; > > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); > > Should cnt also be size_t to match max_vb_buffer_per_queue? > This could also overflow given q->max_num_bufs big enough, so maybe it > would just be better to rewrite to? > > size_t cnt = (q->max_num_bufs > max_vb_buffer_per_queue / 2) ? > max_vb_buffer_per_queue : q->max_num_bufs * 2; > > Or we could just switch to XArray and it would solve this for us. :) I like using existing libraries instead of reinventing the wheel :-) > > + > > + if (cnt >= q->max_num_bufs) > > + goto realloc_failed; > > > > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); > > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); > > if (!tmp) > > goto realloc_failed; > > > > - q->max_num_bufs *= 2; > > + q->max_num_bufs = cnt; > > q->bufs = tmp; > > } > >
On 21/03/2023 11:28, Benjamin Gaignard wrote: > Add module parameter "max_vb_buffer_per_queue" to be able to limit > the number of vb2 buffers store in queue. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > include/media/videobuf2-core.h | 11 +++++++++-- > 2 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index ae9d72f4d181..f4da917ccf3f 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -34,6 +34,8 @@ > static int debug; > module_param(debug, int, 0644); > > +module_param(max_vb_buffer_per_queue, ulong, 0644); There is no MODULE_PARM_DESC here? Please add. I see it is not there for the debug param either, it should be added for that as well. Regards, Hans > + > #define dprintk(q, level, fmt, arg...) \ > do { \ > if (debug >= level) \ > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > struct vb2_buffer *vb; > int ret; > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > - num_buffers = min_t(unsigned int, num_buffers, > - VB2_MAX_FRAME - q->num_buffers); > - > for (buffer = 0; buffer < num_buffers; ++buffer) { > /* Allocate vb2 buffer structures */ > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > /* > * Make sure the requested values and current defaults are sane. > */ > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > /* > * Set this now to ensure that drivers see the correct q->memory value > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > bool no_previous_buffers = !q->num_buffers; > int ret; > > - if (q->num_buffers == VB2_MAX_FRAME) { > - dprintk(q, 1, "maximum number of buffers already allocated\n"); > - return -ENOBUFS; > - } > - > if (no_previous_buffers) { > if (q->waiting_in_dqbuf && *count) { > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > return -EINVAL; > } > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > + num_buffers = *count; > > if (requested_planes && requested_sizes) { > num_planes = requested_planes; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 397dbf6e61e1..b8b34a993e04 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -12,6 +12,7 @@ > #ifndef _MEDIA_VIDEOBUF2_CORE_H > #define _MEDIA_VIDEOBUF2_CORE_H > > +#include <linux/minmax.h> > #include <linux/mm_types.h> > #include <linux/mutex.h> > #include <linux/poll.h> > @@ -48,6 +49,8 @@ struct vb2_fileio_data; > struct vb2_threadio_data; > struct vb2_buffer; > > +static size_t max_vb_buffer_per_queue = 1024; > + > /** > * struct vb2_mem_ops - memory handling/memory allocator operations. > * @alloc: allocate video memory and, optionally, allocator private data, > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * > > if (vb->index >= q->max_num_bufs) { > struct vb2_buffer **tmp; > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); > + > + if (cnt >= q->max_num_bufs) > + goto realloc_failed; > > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); > if (!tmp) > goto realloc_failed; > > - q->max_num_bufs *= 2; > + q->max_num_bufs = cnt; > q->bufs = tmp; > } >
On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote: > On 21/03/2023 11:28, Benjamin Gaignard wrote: > > Add module parameter "max_vb_buffer_per_queue" to be able to limit > > the number of vb2 buffers store in queue. > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > --- > > drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > > include/media/videobuf2-core.h | 11 +++++++++-- > > 2 files changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index ae9d72f4d181..f4da917ccf3f 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -34,6 +34,8 @@ > > static int debug; > > module_param(debug, int, 0644); > > > > +module_param(max_vb_buffer_per_queue, ulong, 0644); > > There is no MODULE_PARM_DESC here? Please add. I see it is not there for > the debug param either, it should be added for that as well. Would this be the right time to consider resource accounting in V4L2 for buffers ? Having a module parameter doesn't sound very useful, an application could easily allocate more buffers by using buffer orphaning (allocating buffers, exporting them as dmabuf objects, and freeing them, which leaves the memory allocated). Repeating allocation cycles up to max_vb_buffer_per_queue will allow allocating an unbounded number of buffers, using all the available system memory. I'd rather not add a module argument that only gives the impression of some kind of safety without actually providing any value. > > + > > #define dprintk(q, level, fmt, arg...) \ > > do { \ > > if (debug >= level) \ > > @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > > struct vb2_buffer *vb; > > int ret; > > > > - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ > > - num_buffers = min_t(unsigned int, num_buffers, > > - VB2_MAX_FRAME - q->num_buffers); > > - > > for (buffer = 0; buffer < num_buffers; ++buffer) { > > /* Allocate vb2 buffer structures */ > > vb = kzalloc(q->buf_struct_size, GFP_KERNEL); > > @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, > > /* > > * Make sure the requested values and current defaults are sane. > > */ > > - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); > > num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); > > - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); > > memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); > > /* > > * Set this now to ensure that drivers see the correct q->memory value > > @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > bool no_previous_buffers = !q->num_buffers; > > int ret; > > > > - if (q->num_buffers == VB2_MAX_FRAME) { > > - dprintk(q, 1, "maximum number of buffers already allocated\n"); > > - return -ENOBUFS; > > - } > > - > > if (no_previous_buffers) { > > if (q->waiting_in_dqbuf && *count) { > > dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); > > @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, > > return -EINVAL; > > } > > > > - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); > > + num_buffers = *count; > > > > if (requested_planes && requested_sizes) { > > num_planes = requested_planes; > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > > index 397dbf6e61e1..b8b34a993e04 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -12,6 +12,7 @@ > > #ifndef _MEDIA_VIDEOBUF2_CORE_H > > #define _MEDIA_VIDEOBUF2_CORE_H > > > > +#include <linux/minmax.h> > > #include <linux/mm_types.h> > > #include <linux/mutex.h> > > #include <linux/poll.h> > > @@ -48,6 +49,8 @@ struct vb2_fileio_data; > > struct vb2_threadio_data; > > struct vb2_buffer; > > > > +static size_t max_vb_buffer_per_queue = 1024; > > + > > /** > > * struct vb2_mem_ops - memory handling/memory allocator operations. > > * @alloc: allocate video memory and, optionally, allocator private data, > > @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * > > > > if (vb->index >= q->max_num_bufs) { > > struct vb2_buffer **tmp; > > + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); > > + > > + if (cnt >= q->max_num_bufs) > > + goto realloc_failed; > > > > - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); > > + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); > > if (!tmp) > > goto realloc_failed; > > > > - q->max_num_bufs *= 2; > > + q->max_num_bufs = cnt; > > q->bufs = tmp; > > } > >
On 5/31/23 10:03, Laurent Pinchart wrote: > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote: >> On 21/03/2023 11:28, Benjamin Gaignard wrote: >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit >>> the number of vb2 buffers store in queue. >>> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>> --- >>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ >>> include/media/videobuf2-core.h | 11 +++++++++-- >>> 2 files changed, 12 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >>> index ae9d72f4d181..f4da917ccf3f 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >>> @@ -34,6 +34,8 @@ >>> static int debug; >>> module_param(debug, int, 0644); >>> >>> +module_param(max_vb_buffer_per_queue, ulong, 0644); >> >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for >> the debug param either, it should be added for that as well. > > Would this be the right time to consider resource accounting in V4L2 for > buffers ? Having a module parameter doesn't sound very useful, an > application could easily allocate more buffers by using buffer orphaning > (allocating buffers, exporting them as dmabuf objects, and freeing them, > which leaves the memory allocated). Repeating allocation cycles up to > max_vb_buffer_per_queue will allow allocating an unbounded number of > buffers, using all the available system memory. I'd rather not add a > module argument that only gives the impression of some kind of safety > without actually providing any value. Does dmabuf itself provide some accounting mechanism? Just wondering. More specific to V4L2: I'm not so sure about this module parameter either. It makes sense to have a check somewhere against ridiculous values (i.e. allocating MAXINT buffers), but that can be a define as well. But otherwise I am fine with allowing applications to allocate buffers until the memory is full. The question is really: what is this parameter supposed to do? The only thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers). I prefer that as a define, to be honest. I think it is perfectly fine for users to try to request more buffers than memory allows. It will just fail in that case, not a problem. And if an application is doing silly things like buffer orphaning, then so what? Is that any different than allocating memory and not freeing it? Eventually it will run out of memory and crash, which is normal. Regards, Hans
On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote: > On 5/31/23 10:03, Laurent Pinchart wrote: > > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote: > >> On 21/03/2023 11:28, Benjamin Gaignard wrote: > >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit > >>> the number of vb2 buffers store in queue. > >>> > >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > >>> --- > >>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > >>> include/media/videobuf2-core.h | 11 +++++++++-- > >>> 2 files changed, 12 insertions(+), 14 deletions(-) > >>> > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > >>> index ae9d72f4d181..f4da917ccf3f 100644 > >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c > >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > >>> @@ -34,6 +34,8 @@ > >>> static int debug; > >>> module_param(debug, int, 0644); > >>> > >>> +module_param(max_vb_buffer_per_queue, ulong, 0644); > >> > >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for > >> the debug param either, it should be added for that as well. > > > > Would this be the right time to consider resource accounting in V4L2 for > > buffers ? Having a module parameter doesn't sound very useful, an > > application could easily allocate more buffers by using buffer orphaning > > (allocating buffers, exporting them as dmabuf objects, and freeing them, > > which leaves the memory allocated). Repeating allocation cycles up to > > max_vb_buffer_per_queue will allow allocating an unbounded number of > > buffers, using all the available system memory. I'd rather not add a > > module argument that only gives the impression of some kind of safety > > without actually providing any value. > > Does dmabuf itself provide some accounting mechanism? Just wondering. > > More specific to V4L2: I'm not so sure about this module parameter either. > It makes sense to have a check somewhere against ridiculous values (i.e. > allocating MAXINT buffers), but that can be a define as well. But otherwise > I am fine with allowing applications to allocate buffers until the memory > is full. > > The question is really: what is this parameter supposed to do? The only > thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers). > > I prefer that as a define, to be honest. > > I think it is perfectly fine for users to try to request more buffers than > memory allows. It will just fail in that case, not a problem. > > And if an application is doing silly things like buffer orphaning, then so > what? Is that any different than allocating memory and not freeing it? > Eventually it will run out of memory and crash, which is normal. Linux provides APIs to account for and limit usage of resources, including memory. A system administrator can prevent rogue processes from starving system resources. The memory consumed by vb2 buffer isn't taken into account, making V4L2 essentially unsafe for untrusted processes. Now, to be fair, there are many reasons why allowing access to v4L2 devices to untrusted applications is a bad idea, and memory consumption is likely not even the worst one. Still, is this something we want to fix, or do we want to consider V4L2 to be priviledged API only ? Right now we can't do so, but with many Linux systems moving towards pipewire, we could possibly have a system daemon isolating untrusted applications from the rest of the system. We may thus not need to fix this in the V4L2 API.
Le 31/05/2023 à 14:39, Laurent Pinchart a écrit : > On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote: >> On 5/31/23 10:03, Laurent Pinchart wrote: >>> On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote: >>>> On 21/03/2023 11:28, Benjamin Gaignard wrote: >>>>> Add module parameter "max_vb_buffer_per_queue" to be able to limit >>>>> the number of vb2 buffers store in queue. >>>>> >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>>> --- >>>>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ >>>>> include/media/videobuf2-core.h | 11 +++++++++-- >>>>> 2 files changed, 12 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >>>>> index ae9d72f4d181..f4da917ccf3f 100644 >>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c >>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >>>>> @@ -34,6 +34,8 @@ >>>>> static int debug; >>>>> module_param(debug, int, 0644); >>>>> >>>>> +module_param(max_vb_buffer_per_queue, ulong, 0644); >>>> There is no MODULE_PARM_DESC here? Please add. I see it is not there for >>>> the debug param either, it should be added for that as well. >>> Would this be the right time to consider resource accounting in V4L2 for >>> buffers ? Having a module parameter doesn't sound very useful, an >>> application could easily allocate more buffers by using buffer orphaning >>> (allocating buffers, exporting them as dmabuf objects, and freeing them, >>> which leaves the memory allocated). Repeating allocation cycles up to >>> max_vb_buffer_per_queue will allow allocating an unbounded number of >>> buffers, using all the available system memory. I'd rather not add a >>> module argument that only gives the impression of some kind of safety >>> without actually providing any value. >> Does dmabuf itself provide some accounting mechanism? Just wondering. >> >> More specific to V4L2: I'm not so sure about this module parameter either. >> It makes sense to have a check somewhere against ridiculous values (i.e. >> allocating MAXINT buffers), but that can be a define as well. But otherwise >> I am fine with allowing applications to allocate buffers until the memory >> is full. >> >> The question is really: what is this parameter supposed to do? The only >> thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers). >> >> I prefer that as a define, to be honest. >> >> I think it is perfectly fine for users to try to request more buffers than >> memory allows. It will just fail in that case, not a problem. >> >> And if an application is doing silly things like buffer orphaning, then so >> what? Is that any different than allocating memory and not freeing it? >> Eventually it will run out of memory and crash, which is normal. > Linux provides APIs to account for and limit usage of resources, > including memory. A system administrator can prevent rogue processes > from starving system resources. The memory consumed by vb2 buffer isn't > taken into account, making V4L2 essentially unsafe for untrusted > processes. > > Now, to be fair, there are many reasons why allowing access to v4L2 > devices to untrusted applications is a bad idea, and memory consumption > is likely not even the worst one. Still, is this something we want to > fix, or do we want to consider V4L2 to be priviledged API only ? Right > now we can't do so, but with many Linux systems moving towards pipewire, > we could possibly have a system daemon isolating untrusted applications > from the rest of the system. We may thus not need to fix this in the > V4L2 API. I'm working in v3 where I'm using Xarray API. Just to be sure to understand you well: I can just remove VB2_MAX_FRAME limit without adding a new one ? >
Hi Benjamin, On Thu, Jun 01, 2023 at 10:03:39AM +0200, Benjamin Gaignard wrote: > Le 31/05/2023 à 14:39, Laurent Pinchart a écrit : > > On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote: > >> On 5/31/23 10:03, Laurent Pinchart wrote: > >>> On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote: > >>>> On 21/03/2023 11:28, Benjamin Gaignard wrote: > >>>>> Add module parameter "max_vb_buffer_per_queue" to be able to limit > >>>>> the number of vb2 buffers store in queue. > >>>>> > >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > >>>>> --- > >>>>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > >>>>> include/media/videobuf2-core.h | 11 +++++++++-- > >>>>> 2 files changed, 12 insertions(+), 14 deletions(-) > >>>>> > >>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > >>>>> index ae9d72f4d181..f4da917ccf3f 100644 > >>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c > >>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > >>>>> @@ -34,6 +34,8 @@ > >>>>> static int debug; > >>>>> module_param(debug, int, 0644); > >>>>> > >>>>> +module_param(max_vb_buffer_per_queue, ulong, 0644); > >>>> There is no MODULE_PARM_DESC here? Please add. I see it is not there for > >>>> the debug param either, it should be added for that as well. > >>> Would this be the right time to consider resource accounting in V4L2 for > >>> buffers ? Having a module parameter doesn't sound very useful, an > >>> application could easily allocate more buffers by using buffer orphaning > >>> (allocating buffers, exporting them as dmabuf objects, and freeing them, > >>> which leaves the memory allocated). Repeating allocation cycles up to > >>> max_vb_buffer_per_queue will allow allocating an unbounded number of > >>> buffers, using all the available system memory. I'd rather not add a > >>> module argument that only gives the impression of some kind of safety > >>> without actually providing any value. > >> Does dmabuf itself provide some accounting mechanism? Just wondering. > >> > >> More specific to V4L2: I'm not so sure about this module parameter either. > >> It makes sense to have a check somewhere against ridiculous values (i.e. > >> allocating MAXINT buffers), but that can be a define as well. But otherwise > >> I am fine with allowing applications to allocate buffers until the memory > >> is full. > >> > >> The question is really: what is this parameter supposed to do? The only > >> thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers). > >> > >> I prefer that as a define, to be honest. > >> > >> I think it is perfectly fine for users to try to request more buffers than > >> memory allows. It will just fail in that case, not a problem. > >> > >> And if an application is doing silly things like buffer orphaning, then so > >> what? Is that any different than allocating memory and not freeing it? > >> Eventually it will run out of memory and crash, which is normal. > > > > Linux provides APIs to account for and limit usage of resources, > > including memory. A system administrator can prevent rogue processes > > from starving system resources. The memory consumed by vb2 buffer isn't > > taken into account, making V4L2 essentially unsafe for untrusted > > processes. > > > > Now, to be fair, there are many reasons why allowing access to v4L2 > > devices to untrusted applications is a bad idea, and memory consumption > > is likely not even the worst one. Still, is this something we want to > > fix, or do we want to consider V4L2 to be priviledged API only ? Right > > now we can't do so, but with many Linux systems moving towards pipewire, > > we could possibly have a system daemon isolating untrusted applications > > from the rest of the system. We may thus not need to fix this in the > > V4L2 API. > > I'm working in v3 where I'm using Xarray API. > > Just to be sure to understand you well: > I can just remove VB2_MAX_FRAME limit without adding a new one ? As long as the code is protected against overflows (e.g. if it uses num_buffers + 1 in some calculations and allows num_buffers to be UINT_MAX, you'll have an issue), it should be fine for the vb2 and V4L2 core. Limiting the number of buffers doesn't really protect against much.
On Wed, May 31, 2023 at 9:39 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote: > > On 5/31/23 10:03, Laurent Pinchart wrote: > > > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote: > > >> On 21/03/2023 11:28, Benjamin Gaignard wrote: > > >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit > > >>> the number of vb2 buffers store in queue. > > >>> > > >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > >>> --- > > >>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > > >>> include/media/videobuf2-core.h | 11 +++++++++-- > > >>> 2 files changed, 12 insertions(+), 14 deletions(-) > > >>> > > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > >>> index ae9d72f4d181..f4da917ccf3f 100644 > > >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c > > >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > >>> @@ -34,6 +34,8 @@ > > >>> static int debug; > > >>> module_param(debug, int, 0644); > > >>> > > >>> +module_param(max_vb_buffer_per_queue, ulong, 0644); > > >> > > >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for > > >> the debug param either, it should be added for that as well. > > > > > > Would this be the right time to consider resource accounting in V4L2 for > > > buffers ? Having a module parameter doesn't sound very useful, an > > > application could easily allocate more buffers by using buffer orphaning > > > (allocating buffers, exporting them as dmabuf objects, and freeing them, > > > which leaves the memory allocated). Repeating allocation cycles up to > > > max_vb_buffer_per_queue will allow allocating an unbounded number of > > > buffers, using all the available system memory. I'd rather not add a > > > module argument that only gives the impression of some kind of safety > > > without actually providing any value. Good point. It's even simpler, just keep opening new vim2m instances and requesting max buffers :). > > > > Does dmabuf itself provide some accounting mechanism? Just wondering. > > > > More specific to V4L2: I'm not so sure about this module parameter either. > > It makes sense to have a check somewhere against ridiculous values (i.e. > > allocating MAXINT buffers), but that can be a define as well. But otherwise > > I am fine with allowing applications to allocate buffers until the memory > > is full. > > > > The question is really: what is this parameter supposed to do? The only > > thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers). > > > > I prefer that as a define, to be honest. > > > > I think it is perfectly fine for users to try to request more buffers than > > memory allows. It will just fail in that case, not a problem. > > > > And if an application is doing silly things like buffer orphaning, then so > > what? Is that any different than allocating memory and not freeing it? > > Eventually it will run out of memory and crash, which is normal. > > Linux provides APIs to account for and limit usage of resources, > including memory. A system administrator can prevent rogue processes > from starving system resources. The memory consumed by vb2 buffer isn't > taken into account, making V4L2 essentially unsafe for untrusted > processes. I agree that proper accounting would be useful, although I wouldn't really make this patch series depend on it, since it's not introducing the loophole in the first place. We had some discussion about this in ChromeOS long ago and we thought it would be really useful for killing browser tabs with big videos, but otherwise using very little regular memory (e.g. via javascript). One challenge with accounting V4L2 allocations is how to count shared DMA-bufs. If one process allocates a V4L2 buffer, exports it to DMA-buf and then sends it to another process that keeps it alive, but frees the V4L2 buffer (and even closes the DMA-buf fd), should that memory be still accounted to it even though it doesn't hold a reference to it anymore? > > Now, to be fair, there are many reasons why allowing access to v4L2 > devices to untrusted applications is a bad idea, and memory consumption > is likely not even the worst one. Still, is this something we want to > fix, or do we want to consider V4L2 to be priviledged API only ? Right > now we can't do so, but with many Linux systems moving towards pipewire, > we could possibly have a system daemon isolating untrusted applications > from the rest of the system. We may thus not need to fix this in the > V4L2 API. > > -- > Regards, > > Laurent Pinchart
Hi Tomasz, On Thu, Jun 08, 2023 at 07:24:29PM +0900, Tomasz Figa wrote: > On Wed, May 31, 2023 at 9:39 PM Laurent Pinchart wrote: > > On Wed, May 31, 2023 at 10:30:36AM +0200, Hans Verkuil wrote: > > > On 5/31/23 10:03, Laurent Pinchart wrote: > > > > On Wed, May 31, 2023 at 08:36:59AM +0200, Hans Verkuil wrote: > > > >> On 21/03/2023 11:28, Benjamin Gaignard wrote: > > > >>> Add module parameter "max_vb_buffer_per_queue" to be able to limit > > > >>> the number of vb2 buffers store in queue. > > > >>> > > > >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > >>> --- > > > >>> drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ > > > >>> include/media/videobuf2-core.h | 11 +++++++++-- > > > >>> 2 files changed, 12 insertions(+), 14 deletions(-) > > > >>> > > > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > > >>> index ae9d72f4d181..f4da917ccf3f 100644 > > > >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c > > > >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > > >>> @@ -34,6 +34,8 @@ > > > >>> static int debug; > > > >>> module_param(debug, int, 0644); > > > >>> > > > >>> +module_param(max_vb_buffer_per_queue, ulong, 0644); > > > >> > > > >> There is no MODULE_PARM_DESC here? Please add. I see it is not there for > > > >> the debug param either, it should be added for that as well. > > > > > > > > Would this be the right time to consider resource accounting in V4L2 for > > > > buffers ? Having a module parameter doesn't sound very useful, an > > > > application could easily allocate more buffers by using buffer orphaning > > > > (allocating buffers, exporting them as dmabuf objects, and freeing them, > > > > which leaves the memory allocated). Repeating allocation cycles up to > > > > max_vb_buffer_per_queue will allow allocating an unbounded number of > > > > buffers, using all the available system memory. I'd rather not add a > > > > module argument that only gives the impression of some kind of safety > > > > without actually providing any value. > > Good point. It's even simpler, just keep opening new vim2m instances > and requesting max buffers :). > > > > > > > Does dmabuf itself provide some accounting mechanism? Just wondering. > > > > > > More specific to V4L2: I'm not so sure about this module parameter either. > > > It makes sense to have a check somewhere against ridiculous values (i.e. > > > allocating MAXINT buffers), but that can be a define as well. But otherwise > > > I am fine with allowing applications to allocate buffers until the memory > > > is full. > > > > > > The question is really: what is this parameter supposed to do? The only > > > thing it does is to sanitize unlikely inputs (e.g. allocating MAXINT buffers). > > > > > > I prefer that as a define, to be honest. > > > > > > I think it is perfectly fine for users to try to request more buffers than > > > memory allows. It will just fail in that case, not a problem. > > > > > > And if an application is doing silly things like buffer orphaning, then so > > > what? Is that any different than allocating memory and not freeing it? > > > Eventually it will run out of memory and crash, which is normal. > > > > Linux provides APIs to account for and limit usage of resources, > > including memory. A system administrator can prevent rogue processes > > from starving system resources. The memory consumed by vb2 buffer isn't > > taken into account, making V4L2 essentially unsafe for untrusted > > processes. > > I agree that proper accounting would be useful, although I wouldn't > really make this patch series depend on it, since it's not introducing > the loophole in the first place. No disagreement here, my concern was about introducing a workaround for the lack of proper memory accounting. I'd like to avoid the workaround, but it doesn't mean memory accounting has to be implement now. > We had some discussion about this in ChromeOS long ago and we thought > it would be really useful for killing browser tabs with big videos, > but otherwise using very little regular memory (e.g. via javascript). > > One challenge with accounting V4L2 allocations is how to count shared > DMA-bufs. If one process allocates a V4L2 buffer, exports it to > DMA-buf and then sends it to another process that keeps it alive, but > frees the V4L2 buffer (and even closes the DMA-buf fd), should that > memory be still accounted to it even though it doesn't hold a > reference to it anymore? I've thought about that too. It's an annoying problem, it should probably be discussed with memory management developers. > > Now, to be fair, there are many reasons why allowing access to v4L2 > > devices to untrusted applications is a bad idea, and memory consumption > > is likely not even the worst one. Still, is this something we want to > > fix, or do we want to consider V4L2 to be priviledged API only ? Right > > now we can't do so, but with many Linux systems moving towards pipewire, > > we could possibly have a system daemon isolating untrusted applications > > from the rest of the system. We may thus not need to fix this in the > > V4L2 API.
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index ae9d72f4d181..f4da917ccf3f 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -34,6 +34,8 @@ static int debug; module_param(debug, int, 0644); +module_param(max_vb_buffer_per_queue, ulong, 0644); + #define dprintk(q, level, fmt, arg...) \ do { \ if (debug >= level) \ @@ -412,10 +414,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, struct vb2_buffer *vb; int ret; - /* Ensure that q->num_buffers+num_buffers is below VB2_MAX_FRAME */ - num_buffers = min_t(unsigned int, num_buffers, - VB2_MAX_FRAME - q->num_buffers); - for (buffer = 0; buffer < num_buffers; ++buffer) { /* Allocate vb2 buffer structures */ vb = kzalloc(q->buf_struct_size, GFP_KERNEL); @@ -801,9 +799,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, /* * Make sure the requested values and current defaults are sane. */ - WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME); num_buffers = max_t(unsigned int, *count, q->min_buffers_needed); - num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME); memset(q->alloc_devs, 0, sizeof(q->alloc_devs)); /* * Set this now to ensure that drivers see the correct q->memory value @@ -919,11 +915,6 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, bool no_previous_buffers = !q->num_buffers; int ret; - if (q->num_buffers == VB2_MAX_FRAME) { - dprintk(q, 1, "maximum number of buffers already allocated\n"); - return -ENOBUFS; - } - if (no_previous_buffers) { if (q->waiting_in_dqbuf && *count) { dprintk(q, 1, "another dup()ped fd is waiting for a buffer\n"); @@ -948,7 +939,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, return -EINVAL; } - num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); + num_buffers = *count; if (requested_planes && requested_sizes) { num_planes = requested_planes; diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 397dbf6e61e1..b8b34a993e04 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -12,6 +12,7 @@ #ifndef _MEDIA_VIDEOBUF2_CORE_H #define _MEDIA_VIDEOBUF2_CORE_H +#include <linux/minmax.h> #include <linux/mm_types.h> #include <linux/mutex.h> #include <linux/poll.h> @@ -48,6 +49,8 @@ struct vb2_fileio_data; struct vb2_threadio_data; struct vb2_buffer; +static size_t max_vb_buffer_per_queue = 1024; + /** * struct vb2_mem_ops - memory handling/memory allocator operations. * @alloc: allocate video memory and, optionally, allocator private data, @@ -1268,12 +1271,16 @@ static inline bool vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer * if (vb->index >= q->max_num_bufs) { struct vb2_buffer **tmp; + int cnt = min(max_vb_buffer_per_queue, q->max_num_bufs * 2); + + if (cnt >= q->max_num_bufs) + goto realloc_failed; - tmp = krealloc_array(q->bufs, q->max_num_bufs * 2, sizeof(*q->bufs), GFP_KERNEL); + tmp = krealloc_array(q->bufs, cnt, sizeof(*q->bufs), GFP_KERNEL); if (!tmp) goto realloc_failed; - q->max_num_bufs *= 2; + q->max_num_bufs = cnt; q->bufs = tmp; }
Add module parameter "max_vb_buffer_per_queue" to be able to limit the number of vb2 buffers store in queue. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- drivers/media/common/videobuf2/videobuf2-core.c | 15 +++------------ include/media/videobuf2-core.h | 11 +++++++++-- 2 files changed, 12 insertions(+), 14 deletions(-)