| 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
| ||
|
Date: Tue, 14 Feb 2023 06:57:01 +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 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,
>
> 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