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<jasow...@redhat.com>
---
   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 0x10000 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
100000" 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

Reply via email to