diff mbox series

[v2,3/8] media: videobuf2: Add a module param to limit vb2 queue buffer storage

Message ID 20230321102855.346732-4-benjamin.gaignard@collabora.com
State New
Headers
Series Add DELETE_BUF ioctl |

Commit Message

Benjamin Gaignard March 21, 2023, 10:28 a.m. UTC 
  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(-)

Comments

kernel test robot March 21, 2023, 5:01 p.m. UTC | #1 
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, &param_ops_##type, &value, perm);		   \
546970bc6afc7fb Rusty Russell   2010-08-11  152  	__MODULE_PARM_TYPE(name, #type)
546970bc6afc7fb Rusty Russell   2010-08-11  153
kernel test robot March 21, 2023, 7:15 p.m. UTC | #2 
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
Ming Qian March 22, 2023, 6:22 a.m. UTC | #3 
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
Tomasz Figa May 19, 2023, 10:19 a.m. UTC | #4 
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
>
Laurent Pinchart May 30, 2023, 5:38 p.m. UTC | #5 
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;
> >  	}
> >
Hans Verkuil May 31, 2023, 6:36 a.m. UTC | #6 
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;
>  	}
>
Laurent Pinchart May 31, 2023, 8:03 a.m. UTC | #7 
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;
> >  	}
> >
Hans Verkuil May 31, 2023, 8:30 a.m. UTC | #8 
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
Laurent Pinchart May 31, 2023, 12:39 p.m. UTC | #9 
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.
Benjamin Gaignard June 1, 2023, 8:03 a.m. UTC | #10 
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 ?

>
Laurent Pinchart June 1, 2023, 8:34 a.m. UTC | #11 
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.
Tomasz Figa June 8, 2023, 10:24 a.m. UTC | #12 
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
Laurent Pinchart June 8, 2023, 10:42 a.m. UTC | #13 
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 mbox series

Patch

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;
 	}