From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-04 18:55:38
On Thu, 4 Feb 2021 10:51:59 -0800 Jakub Kicinski wrote:
On Thu, 4 Feb 2021 15:02:30 +0600 Sabyrzhan Tasbolatov wrote:
quoted
Replaced kzalloc() with kmalloc(), there is no need for zeroed-out
memory for simple void *kbuf.
There is no need for zeroed-out memory because it's immediately
overwritten by copy_from_iter...
Also if you don't mind please wait a week until the fixes get merged
back into net-next and then repost. Otherwise this patch will not apply
cleanly. (Fixes are merged into a different tree than cleanups)
Probably not enough.
qrtr_endpoint_post() will later attempt a netdev_alloc_skb() which will need
some extra space (for struct skb_shared_info)
Do we really expect to accept huge lengths here ?
+
kbuf = kzalloc(len, GFP_KERNEL);
if (!kbuf)
return -ENOMEM;
Sorry for late response but I couldnt find any reference to the max
length of incoming data for qrtr TUN interface.
qrtr_endpoint_post() will later attempt a netdev_alloc_skb() which will need
some extra space (for struct skb_shared_info)
Thanks, you're right, qrtr_endpoint_post() will alloc another slab buffer.
We can check the length of skb allocation but we need to do following:
int qrtr_endpoint_post(.., const void *data, size_t len)
{
..
when QRTR_PROTO_VER_1:
hdrlen = sizeof(*data);
when QRTR_PROTO_VER_2:
hdrlen = sizeof(*data) + data->optlen;
len = (KMALLOC_MAX_SIZE - hdrlen) % data->size;
skb = netdev_alloc_skb(NULL, len);
..
skb_put_data(skb, data + hdrlen, size);
So it requires refactoring as in qrtr_tun_write_iter() we just allocate and
pass it to qrtr_endpoint_post() and there
we need to do len calculation as above *before* netdev_alloc_skb(NULL, len).
Perhaps there is a nicer solution though.
From: Eric Dumazet <hidden> Date: 2021-02-22 08:45:59
On 2/21/21 1:39 PM, Sabyrzhan Tasbolatov wrote:
quoted
Do we really expect to accept huge lengths here ?
Sorry for late response but I couldnt find any reference to the max
length of incoming data for qrtr TUN interface.
quoted
qrtr_endpoint_post() will later attempt a netdev_alloc_skb() which will need
some extra space (for struct skb_shared_info)
Thanks, you're right, qrtr_endpoint_post() will alloc another slab buffer.
We can check the length of skb allocation but we need to do following:
int qrtr_endpoint_post(.., const void *data, size_t len)
{
..
when QRTR_PROTO_VER_1:
hdrlen = sizeof(*data);
when QRTR_PROTO_VER_2:
hdrlen = sizeof(*data) + data->optlen;
len = (KMALLOC_MAX_SIZE - hdrlen) % data->size;
skb = netdev_alloc_skb(NULL, len);
..
skb_put_data(skb, data + hdrlen, size);
So it requires refactoring as in qrtr_tun_write_iter() we just allocate and
pass it to qrtr_endpoint_post() and there
we need to do len calculation as above *before* netdev_alloc_skb(NULL, len).
Perhaps there is a nicer solution though.
A protocol requiring contiguous physical memory allocations of up to KMALLOC_MAX_SIZE
bytes would be really unreliable.
I suggest we simply limit the allocations to 64KB, unless qrtr maintainers shout,
or start implementing scatter gather.