Re: [PATCH] net/qrtr: restrict length in qrtr_tun_write_iter()

2021-02-22 Thread Eric Dumazet



On 2/21/21 1:39 PM, Sabyrzhan Tasbolatov wrote:
>> 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.
> 
>> 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.







Re: [PATCH] net/qrtr: restrict length in qrtr_tun_write_iter()

2021-02-21 Thread Sabyrzhan Tasbolatov
> 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.

> 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.