Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)
On 2018年02月09日 03:09, David Miller wrote:
From: Jason Wang
Date: Thu, 8 Feb 2018 11:59:25 +0800
We need limit the maximum size of queue, otherwise it may cause
several side effects e.g slab will warn when the size exceeds
KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
tries to limit it to 64K. This value could be revisited if we found a
real case that needs more.
Reported-by: syzbot+e4d4f9ddd42955397...@syzkaller.appspotmail.com
Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
Signed-off-by: Jason Wang
...
@@ -466,6 +468,8 @@ static inline int ptr_ring_consume_batched_bh(struct
ptr_ring *r,
static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
{
+ if (size > PTR_RING_MAX_ALLOC)
+ return NULL;
return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
}
This doesn't limit the allocation to 64K. It limits it to 64K * sizeof(void *).
Right, will fix this.
Thanks
Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)
On 2018年02月08日 23:50, Michael S. Tsirkin wrote:
On Thu, Feb 08, 2018 at 03:11:22PM +0800, Jason Wang wrote:
On 2018年02月08日 12:52, Michael S. Tsirkin wrote:
On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
We need limit the maximum size of queue, otherwise it may cause
several side effects e.g slab will warn when the size exceeds
KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
tries to limit it to 64K. This value could be revisited if we found a
real case that needs more.
Reported-by:syzbot+e4d4f9ddd42955397...@syzkaller.appspotmail.com
Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
Signed-off-by: Jason Wang
---
include/linux/ptr_ring.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 2af71a7..5858d48 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -44,6 +44,8 @@ struct ptr_ring {
void **queue;
};
Seems like a weird location for a define. Either put defines on
top of the file, or near where they are used. I prefer the
second option.
Ok.
+#define PTR_RING_MAX_ALLOC 65536
+
I guess it's an arbitrary number. Seems like a sufficiently large one,
but pls add a comment so readers don't wonder. And please explain what
it does:
/* Callers can create ptr_ring structures with userspace-supplied
* parameters. This sets a limit on the size to make that usecase
* safe. If you ever change this, make sure to audit all callers.
*/
Also I think we should generally use either hex 0x1 or (1 << 16).
I agree the number is arbitrary, so I still prefer the KMALLOC_MAX_SIZE
especially consider it was used by pfifo_fast now. Try to limit it to an
arbitrary may break lots of exist setups. E.g just google "txqueuelen
10" can give me a lots of search results.
We can do any kind of optimization on top but not for -net now.
Thanks
Interesting. I have an idea for fixing this, but maybe
for now KMALLOC_MAX_SIZE does make sense. It's unfortunate that
this value is architecture dependent.
The patch still needs code comments though, and fix the math to
use the proper size.
Yes.
Thanks
Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)
From: Jason Wang
Date: Thu, 8 Feb 2018 11:59:25 +0800
> We need limit the maximum size of queue, otherwise it may cause
> several side effects e.g slab will warn when the size exceeds
> KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
> tries to limit it to 64K. This value could be revisited if we found a
> real case that needs more.
>
> Reported-by: syzbot+e4d4f9ddd42955397...@syzkaller.appspotmail.com
> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> Signed-off-by: Jason Wang
...
> @@ -466,6 +468,8 @@ static inline int ptr_ring_consume_batched_bh(struct
> ptr_ring *r,
>
> static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t
> gfp)
> {
> + if (size > PTR_RING_MAX_ALLOC)
> + return NULL;
> return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
> }
This doesn't limit the allocation to 64K. It limits it to 64K * sizeof(void *).
Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)
On Thu, Feb 08, 2018 at 03:11:22PM +0800, Jason Wang wrote:
>
>
> On 2018年02月08日 12:52, Michael S. Tsirkin wrote:
> > On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
> > > We need limit the maximum size of queue, otherwise it may cause
> > > several side effects e.g slab will warn when the size exceeds
> > > KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
> > > tries to limit it to 64K. This value could be revisited if we found a
> > > real case that needs more.
> > >
> > > Reported-by: syzbot+e4d4f9ddd42955397...@syzkaller.appspotmail.com
> > > Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> > > Signed-off-by: Jason Wang
> > > ---
> > > include/linux/ptr_ring.h | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > > index 2af71a7..5858d48 100644
> > > --- a/include/linux/ptr_ring.h
> > > +++ b/include/linux/ptr_ring.h
> > > @@ -44,6 +44,8 @@ struct ptr_ring {
> > > void **queue;
> > > };
> > Seems like a weird location for a define. Either put defines on
> > top of the file, or near where they are used. I prefer the
> > second option.
>
> Ok.
>
> >
> > > +#define PTR_RING_MAX_ALLOC 65536
> > > +
> > I guess it's an arbitrary number. Seems like a sufficiently large one,
> > but pls add a comment so readers don't wonder. And please explain what
> > it does:
> >
> > /* Callers can create ptr_ring structures with userspace-supplied
> > * parameters. This sets a limit on the size to make that usecase
> > * safe. If you ever change this, make sure to audit all callers.
> > */
> >
> > Also I think we should generally use either hex 0x1 or (1 << 16).
>
> I agree the number is arbitrary, so I still prefer the KMALLOC_MAX_SIZE
> especially consider it was used by pfifo_fast now. Try to limit it to an
> arbitrary may break lots of exist setups. E.g just google "txqueuelen
> 10" can give me a lots of search results.
>
> We can do any kind of optimization on top but not for -net now.
>
> Thanks
Interesting. I have an idea for fixing this, but maybe
for now KMALLOC_MAX_SIZE does make sense. It's unfortunate that
this value is architecture dependent.
The patch still needs code comments though, and fix the math to
use the proper size.
> >
> > > /* Note: callers invoking this in a loop must use a compiler barrier,
> > >* for example cpu_relax().
> > >*
> > > @@ -466,6 +468,8 @@ static inline int ptr_ring_consume_batched_bh(struct
> > > ptr_ring *r,
> > > static inline void **__ptr_ring_init_queue_alloc(unsigned int size,
> > > gfp_t gfp)
> > > {
> > > + if (size > PTR_RING_MAX_ALLOC)
> > > + return NULL;
> > > return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
> > > }
> > > --
> > > 2.7.4
Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)
On 2018年02月08日 12:52, Michael S. Tsirkin wrote:
On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
We need limit the maximum size of queue, otherwise it may cause
several side effects e.g slab will warn when the size exceeds
KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
tries to limit it to 64K. This value could be revisited if we found a
real case that needs more.
Reported-by: syzbot+e4d4f9ddd42955397...@syzkaller.appspotmail.com
Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
Signed-off-by: Jason Wang
---
include/linux/ptr_ring.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 2af71a7..5858d48 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -44,6 +44,8 @@ struct ptr_ring {
void **queue;
};
Seems like a weird location for a define. Either put defines on
top of the file, or near where they are used. I prefer the
second option.
Ok.
+#define PTR_RING_MAX_ALLOC 65536
+
I guess it's an arbitrary number. Seems like a sufficiently large one,
but pls add a comment so readers don't wonder. And please explain what
it does:
/* Callers can create ptr_ring structures with userspace-supplied
* parameters. This sets a limit on the size to make that usecase
* safe. If you ever change this, make sure to audit all callers.
*/
Also I think we should generally use either hex 0x1 or (1 << 16).
I agree the number is arbitrary, so I still prefer the KMALLOC_MAX_SIZE
especially consider it was used by pfifo_fast now. Try to limit it to an
arbitrary may break lots of exist setups. E.g just google "txqueuelen
10" can give me a lots of search results.
We can do any kind of optimization on top but not for -net now.
Thanks
/* Note: callers invoking this in a loop must use a compiler barrier,
* for example cpu_relax().
*
@@ -466,6 +468,8 @@ static inline int ptr_ring_consume_batched_bh(struct
ptr_ring *r,
static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
{
+ if (size > PTR_RING_MAX_ALLOC)
+ return NULL;
return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
}
--
2.7.4
Re: [PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)
On Thu, Feb 08, 2018 at 11:59:25AM +0800, Jason Wang wrote:
> We need limit the maximum size of queue, otherwise it may cause
> several side effects e.g slab will warn when the size exceeds
> KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
> tries to limit it to 64K. This value could be revisited if we found a
> real case that needs more.
>
> Reported-by: syzbot+e4d4f9ddd42955397...@syzkaller.appspotmail.com
> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
> Signed-off-by: Jason Wang
> ---
> include/linux/ptr_ring.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 2af71a7..5858d48 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -44,6 +44,8 @@ struct ptr_ring {
> void **queue;
> };
>
Seems like a weird location for a define. Either put defines on
top of the file, or near where they are used. I prefer the
second option.
> +#define PTR_RING_MAX_ALLOC 65536
> +
I guess it's an arbitrary number. Seems like a sufficiently large one,
but pls add a comment so readers don't wonder. And please explain what
it does:
/* Callers can create ptr_ring structures with userspace-supplied
* parameters. This sets a limit on the size to make that usecase
* safe. If you ever change this, make sure to audit all callers.
*/
Also I think we should generally use either hex 0x1 or (1 << 16).
> /* Note: callers invoking this in a loop must use a compiler barrier,
> * for example cpu_relax().
> *
> @@ -466,6 +468,8 @@ static inline int ptr_ring_consume_batched_bh(struct
> ptr_ring *r,
>
> static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t
> gfp)
> {
> + if (size > PTR_RING_MAX_ALLOC)
> + return NULL;
> return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
> }
>
> --
> 2.7.4
[PATCH net V3 2/2] ptr_ring: fail on large queue size (>64K)
We need limit the maximum size of queue, otherwise it may cause
several side effects e.g slab will warn when the size exceeds
KMALLOC_MAX_SIZE. Using KMALLOC_MAX_SIZE still looks too so this patch
tries to limit it to 64K. This value could be revisited if we found a
real case that needs more.
Reported-by: syzbot+e4d4f9ddd42955397...@syzkaller.appspotmail.com
Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers")
Signed-off-by: Jason Wang
---
include/linux/ptr_ring.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 2af71a7..5858d48 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -44,6 +44,8 @@ struct ptr_ring {
void **queue;
};
+#define PTR_RING_MAX_ALLOC 65536
+
/* Note: callers invoking this in a loop must use a compiler barrier,
* for example cpu_relax().
*
@@ -466,6 +468,8 @@ static inline int ptr_ring_consume_batched_bh(struct
ptr_ring *r,
static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp)
{
+ if (size > PTR_RING_MAX_ALLOC)
+ return NULL;
return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO);
}
--
2.7.4
