| 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: <CAMZdPi96dZV0J_6U-mH5eCquWycSQLPvoz6JX1BHWn0eQJyeDA@mail.gmail.com>
Date: Tue, 8 Nov 2022 13:14:16 +0100
From: Loic Poulain <loic.poulain@...aro.org>
To: haozhe.chang@...iatek.com
Cc: Chandrashekar Devegowda <chandrashekar.devegowda@...el.com>,
Intel Corporation <linuxwwan@...el.com>,
Chiranjeevi Rapolu <chiranjeevi.rapolu@...ux.intel.com>,
Liu Haijun <haijun.liu@...iatek.com>,
M Chetan Kumar <m.chetan.kumar@...ux.intel.com>,
Ricardo Martinez <ricardo.martinez@...ux.intel.com>,
Sergey Ryazanov <ryazanov.s.a@...il.com>,
Johannes Berg <johannes@...solutions.net>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Matthias Brugger <matthias.bgg@...il.com>,
"open list:MEDIATEK T7XX 5G WWAN MODEM DRIVER"
<netdev@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>, lambert.wang@...iatek.com,
xiayu.zhang@...iatek.com, hua.yang@...iatek.com,
srv_heupstream@...iatek.com
Subject: Re: [PATCH v2] wwan: core: Support slicing in port TX flow of WWAN subsystem
Hi Haozhe,
On Tue, 8 Nov 2022 at 11:54, <haozhe.chang@...iatek.com> wrote:
>
> From: haozhe chang <haozhe.chang@...iatek.com>
>
> wwan_port_fops_write inputs the SKB parameter to the TX callback of
> the WWAN device driver. However, the WWAN device (e.g., t7xx) may
> have an MTU less than the size of SKB, causing the TX buffer to be
> sliced and copied once more in the WWAN device driver.
>
> This patch implements the slicing in the WWAN subsystem and gives
> the WWAN devices driver the option to slice(by chunk) or not. By
> doing so, the additional memory copy is reduced.
>
> Meanwhile, this patch gives WWAN devices driver the option to reserve
> headroom in SKB for the device-specific metadata.
>
> Signed-off-by: haozhe chang <haozhe.chang@...iatek.com>
>
> ---
> Changes in v2
> -send fragments to device driver by skb frag_list.
> ---
> drivers/net/wwan/t7xx/t7xx_port_wwan.c | 42 ++++++++++-------
> drivers/net/wwan/wwan_core.c | 65 ++++++++++++++++++++------
> include/linux/wwan.h | 5 +-
> 3 files changed, 80 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> index 33931bfd78fd..74fa58575d5a 100644
> --- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> @@ -54,13 +54,13 @@ static void t7xx_port_ctrl_stop(struct wwan_port *port)
[...]
> static const struct wwan_port_ops wwan_ops = {
> .start = t7xx_port_ctrl_start,
> .stop = t7xx_port_ctrl_stop,
> .tx = t7xx_port_ctrl_tx,
> + .needed_headroom = t7xx_port_tx_headroom,
> + .tx_chunk_len = t7xx_port_tx_chunk_len,
Can you replace 'chunk' with 'frag' everywhere?
> };
>
> static int t7xx_port_wwan_init(struct t7xx_port *port)
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index 62e9f7d6c9fe..ed78471f9e38 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -20,7 +20,7 @@
> #include <uapi/linux/wwan.h>
>
> /* Maximum number of minors in use */
> -#define WWAN_MAX_MINORS (1 << MINORBITS)
> +#define WWAN_MAX_MINORS BIT(MINORBITS)
>
> static DEFINE_MUTEX(wwan_register_lock); /* WWAN device create|remove lock */
> static DEFINE_IDA(minors); /* minors for WWAN port chardevs */
> @@ -67,6 +67,8 @@ struct wwan_device {
> * @rxq: Buffer inbound queue
> * @waitqueue: The waitqueue for port fops (read/write/poll)
> * @data_lock: Port specific data access serialization
> + * @headroom_len: SKB reserved headroom size
> + * @chunk_len: Chunk len to split packet
> * @at_data: AT port specific data
> */
> struct wwan_port {
> @@ -79,6 +81,8 @@ struct wwan_port {
> struct sk_buff_head rxq;
> wait_queue_head_t waitqueue;
> struct mutex data_lock; /* Port specific data access serialization */
> + size_t headroom_len;
> + size_t chunk_len;
> union {
> struct {
> struct ktermios termios;
> @@ -550,8 +554,13 @@ static int wwan_port_op_start(struct wwan_port *port)
> }
>
> /* If port is already started, don't start again */
> - if (!port->start_count)
> + if (!port->start_count) {
> ret = port->ops->start(port);
> + if (port->ops->tx_chunk_len)
> + port->chunk_len = port->ops->tx_chunk_len(port);
So, maybe frag len and headroom should be parameters of
wwan_create_port() instead of port ops, as we really need this info
only once.
> + if (port->ops->needed_headroom)
> + port->headroom_len = port->ops->needed_headroom(port);
> + }
>
> if (!ret)
> port->start_count++;
> @@ -698,30 +707,56 @@ static ssize_t wwan_port_fops_read(struct file *filp, char __user *buf,
> static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf,
> size_t count, loff_t *offp)
> {
> + size_t len, chunk_len, offset, allowed_chunk_len;
> + struct sk_buff *skb, *head = NULL, *tail = NULL;
> struct wwan_port *port = filp->private_data;
> - struct sk_buff *skb;
> int ret;
>
> ret = wwan_wait_tx(port, !!(filp->f_flags & O_NONBLOCK));
> if (ret)
> return ret;
>
> - skb = alloc_skb(count, GFP_KERNEL);
> - if (!skb)
> - return -ENOMEM;
> + allowed_chunk_len = port->chunk_len ? port->chunk_len : count;
I would suggest making port->chunk_len (frag_len) always valid, by
setting it to -1 (MAX size_t) when creating a port without frag_len
requirement.
> + for (offset = 0; offset < count; offset += chunk_len) {
> + chunk_len = min(count - offset, allowed_chunk_len);
> + len = chunk_len + port->headroom_len;
> + skb = alloc_skb(len, GFP_KERNEL);
That works but would prefer a simpler solution like:
do {
len = min(port->frag_len, remain);
skb = alloc_skb(len + port->needed_headroom; GFP_KERNEL);
[...]
copy_from_user(skb_put(skb, len), buf + count - remain)
} while ((remain -= len));
> + if (!skb) {
> + ret = -ENOMEM;
> + goto freeskb;
> + }
> + skb_reserve(skb, port->headroom_len);
> +
> + if (!head) {
> + head = skb;
> + } else if (!tail) {
> + skb_shinfo(head)->frag_list = skb;
> + tail = skb;
> + } else {
> + tail->next = skb;
> + tail = skb;
> + }
>
> - if (copy_from_user(skb_put(skb, count), buf, count)) {
> - kfree_skb(skb);
> - return -EFAULT;
> - }
> + if (copy_from_user(skb_put(skb, chunk_len), buf + offset, chunk_len)) {
> + ret = -EFAULT;
> + goto freeskb;
> + }
>
> - ret = wwan_port_op_tx(port, skb, !!(filp->f_flags & O_NONBLOCK));
> - if (ret) {
> - kfree_skb(skb);
> - return ret;
> + if (skb != head) {
> + head->data_len += skb->len;
> + head->len += skb->len;
> + head->truesize += skb->truesize;
> + }
> }
>
> - return count;
> + if (head) {
How head can be null here?
> + ret = wwan_port_op_tx(port, head, !!(filp->f_flags & O_NONBLOCK));
> + if (!ret)
> + return count;
> + }
> +freeskb:
> + kfree_skb(head);
> + return ret;
> }
>
> static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 5ce2acf444fb..bdeeef59bbfd 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h
> @@ -46,6 +46,8 @@ struct wwan_port;
> * @tx: Non-blocking routine that sends WWAN port protocol data to the device.
> * @tx_blocking: Optional blocking routine that sends WWAN port protocol data
> * to the device.
> + * @needed_headroom: Optional routine that sets reserve headroom of skb.
> + * @tx_chunk_len: Optional routine that sets chunk len to split.
> * @tx_poll: Optional routine that sets additional TX poll flags.
> *
> * The wwan_port_ops structure contains a list of low-level operations
> @@ -58,6 +60,8 @@ struct wwan_port_ops {
>
> /* Optional operations */
> int (*tx_blocking)(struct wwan_port *port, struct sk_buff *skb);
> + size_t (*needed_headroom)(struct wwan_port *port);
> + size_t (*tx_chunk_len)(struct wwan_port *port);
As said above, maybe move that as variables, or parameter of wwan_create_port.
Regards,
Loic
Powered by blists - more mailing lists