| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6d26bec0-689c-2130-7e95-ff5fd2383de3@quicinc.com>
Date: Fri, 17 Feb 2023 20:10:33 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Maciej Żenczykowski <maze@...gle.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<quic_ppratap@...cinc.com>, <quic_jackp@...cinc.com>
Subject: Re: [RFC] usb: gadget: ncm: Add support to configure wMaSegmentSize
On 2/14/2023 6:57 AM, Krishna Kurapati PSSNV wrote:
>
>
> On 2/14/2023 3:17 AM, Maciej Żenczykowski wrote:
>> This isn't a review per say - just some loose comments.
>>
>> On Sun, Feb 12, 2023 at 9:57 AM Krishna Kurapati
>> <quic_kriskura@...cinc.com> wrote:
>>> Currently the NCM driver restricts wMasxSegmentSize that indicates
>>
>> there's a number of spelling mistakes, here, and in the commit title
>>
>>> the datagram size coming from network layer to 1514. However the
>>> spec doesn't have any limitation.
>>>
>>> Add support to configure this value before configfs symlink is
>>> created. Also since the NTB Out/In buffer sizes are fixed at 16384
>>> bytes, limit the segment size to an upper cap of 8192 bytes so that
>>> at least 2 packets can be aggregated.
>>
>> I've experimented with increasing mtu to boost performance before
>> (have some half-baked patches/scripts somewhere).
>> And while it did improve point-to-point performance, it wasn't
>> actually useful for any real world use cases,
>> as internet mtu is simply never above 1500.
>>
>> Note that you cannot simply receive, aggregate (lro/gro) and forward
>> aggregated packets without splitting them back up.
> One more query on this:
>
> Would this problem come up if the MTU and segment size are consistent as
> well ? i.e., if the datagrams present in NTB are of size <= MTU so that
> they can be given back safely to network stack without any further
> aggregation/splitup before wrap/unwrap call at usb layer ?
>
> Regards,
> Krishna,
>
Hi Maciej Żenczykowski,
Any thoughts on this ?
I believe if we modify the ncm_opts->net->mtu to the datagram
length (excluding the header) before registering net device in bind
call, we wouldn't need to split/aggregate obtained data before giving it
to network layer right as they would definitely be of size <= MTU of the
net device ? I tried it out and no issue came up in iperf test.
Thanks,
Krishna,
>>
>> A change like this to be useful would require negotiating some sort of
>> gso capabilities between the two devices
>> (and thus extending the NCM standard). I've been meaning to do
>> this... but time...
>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
>>> ---
>>> drivers/usb/gadget/function/f_ncm.c | 55 +++++++++++++++++++++++++++++
>>> drivers/usb/gadget/function/u_ncm.h | 1 +
>>> 2 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_ncm.c
>>> b/drivers/usb/gadget/function/f_ncm.c
>>> index 424bb3b666db..1969e276017f 100644
>>> --- a/drivers/usb/gadget/function/f_ncm.c
>>> +++ b/drivers/usb/gadget/function/f_ncm.c
>>> @@ -118,6 +118,12 @@ static inline unsigned ncm_bitrate(struct
>>> usb_gadget *g)
>>> /* Delay for the transmit to wait before sending an unfilled NTB
>>> frame. */
>>> #define TX_TIMEOUT_NSECS 300000
>>>
>>> +/*
>>> + * Currently the max NTB Buffer size is set to 16384. For atleast 2
>>> packets
>>> + * to be aggregated, the size of datagram must at max be 8192.
>>> + */
>>> +#define MAX_DATAGRAM_SIZE 8192
>>
>> From what I recall, there's a fair bit of overhead, and 8192 x2
>> doesn't actually fit in 16384...
>> That said... is it reasonable to require 2 to fit? why? what's wrong
>> with 15000?
>>
>>> +
>>> #define FORMATS_SUPPORTED (USB_CDC_NCM_NTB16_SUPPORTED | \
>>> USB_CDC_NCM_NTB32_SUPPORTED)
>>>
>>> @@ -1440,6 +1446,7 @@ static int ncm_bind(struct usb_configuration
>>> *c, struct usb_function *f)
>>> */
>>> if (!ncm_opts->bound) {
>>> mutex_lock(&ncm_opts->lock);
>>> + ncm_opts->net->mtu = (ncm_opts->max_segment_size - 14);
>>
>> this should use a constant, ETH_HLEN probably
>>
>>> gether_set_gadget(ncm_opts->net, cdev->gadget);
>>> status = gether_register_netdev(ncm_opts->net);
>>> mutex_unlock(&ncm_opts->lock);
>>> @@ -1484,6 +1491,8 @@ static int ncm_bind(struct usb_configuration
>>> *c, struct usb_function *f)
>>>
>>> status = -ENODEV;
>>>
>>> + ecm_desc.wMaxSegmentSize = ncm_opts->max_segment_size;
>>> +
>>
>> Curious... wasn't this set previously?
>>
>>> /* allocate instance-specific endpoints */
>>> ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_in_desc);
>>> if (!ep)
>>> @@ -1586,11 +1595,56 @@ USB_ETHERNET_CONFIGFS_ITEM_ATTR_QMULT(ncm);
>>> /* f_ncm_opts_ifname */
>>> USB_ETHERNET_CONFIGFS_ITEM_ATTR_IFNAME(ncm);
>>>
>>> +static ssize_t ncm_opts_max_segment_size_show(struct config_item *item,
>>> + char *page)
>>> +{
>>> + struct f_ncm_opts *opts = to_f_ncm_opts(item);
>>> + u32 segment_size;
>>> +
>>> + mutex_lock(&opts->lock);
>>> + segment_size = opts->max_segment_size;
>>> + mutex_unlock(&opts->lock);
>>> +
>>> + return sprintf(page, "%u\n", segment_size);
>>> +}
>>> +
>>> +static ssize_t ncm_opts_max_segment_size_store(struct config_item
>>> *item,
>>> + const char *page,
>>> size_t len)
>>> +{
>>> + struct f_ncm_opts *opts = to_f_ncm_opts(item);
>>> + int ret;
>>> + u32 segment_size;
>>> +
>>> + mutex_lock(&opts->lock);
>>> + if (opts->refcnt) {
>>> + ret = -EBUSY;
>>> + goto out;
>>> + }
>>> +
>>> + ret = kstrtou32(page, 0, &segment_size);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + if (segment_size > MAX_DATAGRAM_SIZE) {
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> +
>>> + opts->max_segment_size = segment_size;
>>> + ret = len;
>>> +out:
>>> + mutex_unlock(&opts->lock);
>>> + return ret;
>>> +}
>>> +
>>> +CONFIGFS_ATTR(ncm_opts_, max_segment_size);
>>> +
>>> static struct configfs_attribute *ncm_attrs[] = {
>>> &ncm_opts_attr_dev_addr,
>>> &ncm_opts_attr_host_addr,
>>> &ncm_opts_attr_qmult,
>>> &ncm_opts_attr_ifname,
>>> + &ncm_opts_attr_max_segment_size,
>>> NULL,
>>> };
>>>
>>> @@ -1633,6 +1687,7 @@ static struct usb_function_instance
>>> *ncm_alloc_inst(void)
>>> kfree(opts);
>>> return ERR_CAST(net);
>>> }
>>> + opts->max_segment_size = cpu_to_le16(ETH_FRAME_LEN);
>>> INIT_LIST_HEAD(&opts->ncm_os_desc.ext_prop);
>>>
>>> descs[0] = &opts->ncm_os_desc;
>>> diff --git a/drivers/usb/gadget/function/u_ncm.h
>>> b/drivers/usb/gadget/function/u_ncm.h
>>> index 5408854d8407..fab99d997476 100644
>>> --- a/drivers/usb/gadget/function/u_ncm.h
>>> +++ b/drivers/usb/gadget/function/u_ncm.h
>>> @@ -31,6 +31,7 @@ struct f_ncm_opts {
>>> */
>>> struct mutex lock;
>>> int refcnt;
>>> + u32 max_segment_size;
>>> };
>>>
>>> #endif /* U_NCM_H */
>>> --
>>> 2.39.0
>>>
Powered by blists - more mailing lists