| 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: <ZR6vxaot4AP7FXTg@kernel.org>
Date: Thu, 5 Oct 2023 14:44:53 +0200
From: Simon Horman <horms@...nel.org>
To: Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, netdev@...r.kernel.org,
vadim.fedorenko@...ux.dev, lkp@...el.com,
Sean Tranchetti <quic_stranche@...cinc.com>
Subject: Re: [PATCH net-next v3] net: qualcomm: rmnet: Add side band flow
control support
On Wed, Oct 04, 2023 at 01:43:20PM -0700, Subash Abhinov Kasiviswanathan wrote:
> Individual rmnet devices map to specific network types such as internet,
> multimedia messaging services, IP multimedia subsystem etc. Each of
> these network types may support varying quality of service for different
> bearers or traffic types.
>
> The physical device interconnect to radio hardware may support a
> higher data rate than what is actually supported by the radio network.
> Any packets transmitted to the radio hardware which exceed the radio
> network data rate limit maybe dropped. This patch tries to minimize the
> loss of packets by adding support for bearer level flow control within a
> rmnet device by ensuring that the packets transmitted do not exceed the
> limit allowed by the radio network.
>
> In order to support multiple bearers, rmnet must be created as a
> multiqueue TX netdevice. Radio hardware communicates the supported
> bearer information for a given network via side band signalling.
> Consider the following mapping -
>
> IPv4 UDP port 1234 - Mark 0x1001 - Queue 1
> IPv6 TCP port 2345 - Mark 0x2001 - Queue 2
>
> iptables can be used to install filters which mark packets matching these
> specific traffic patterns and the RMNET_QUEUE_MAPPING_ADD operation can
> then be to install the mapping of the mark to the specific txqueue.
>
> If the traffic limit is exceeded for a particular bearer, radio hardware
> would notify that the bearer cannot accept more packets and the
> corresponding txqueue traffic can be stopped using RMNET_QUEUE_DISABLE.
>
> Conversely, if radio hardware can send more traffic for a particular
> bearer, RMNET_QUEUE_ENABLE can be used to allow traffic on that
> particular txqueue. RMNET_QUEUE_MAPPING_REMOVE can be used to remove the
> mark to queue mapping in case the radio network doesn't support that
> particular bearer any longer.
>
> Signed-off-by: Sean Tranchetti <quic_stranche@...cinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>
Hi Subash and Sean,
a few comments on error handling from my side.
...
> @@ -88,6 +90,66 @@ static int rmnet_register_real_device(struct net_device *real_dev,
> return 0;
> }
>
> +static int rmnet_update_queue_map(struct net_device *dev, u8 operation,
> + u8 txqueue, u32 mark,
> + struct netlink_ext_ack *extack)
> +{
> + struct rmnet_priv *priv = netdev_priv(dev);
> + struct netdev_queue *q;
> + void *p;
> + u8 txq;
> +
> + if (unlikely(txqueue >= dev->num_tx_queues)) {
> + NL_SET_ERR_MSG_MOD(extack, "invalid txqueue");
> + return -EINVAL;
> + }
> +
> + switch (operation) {
> + case RMNET_QUEUE_MAPPING_ADD:
> + p = xa_store(&priv->queue_map, mark, xa_mk_value(txqueue),
> + GFP_ATOMIC);
> + if (xa_is_err(p)) {
> + NL_SET_ERR_MSG_MOD(extack, "unable to add mapping");
> + return -EINVAL;
> + }
> + break;
> + case RMNET_QUEUE_MAPPING_REMOVE:
> + p = xa_erase(&priv->queue_map, mark);
> + if (xa_is_err(p)) {
> + NL_SET_ERR_MSG_MOD(extack, "unable to remove mapping");
> + return -EINVAL;
> + }
> + break;
> + case RMNET_QUEUE_ENABLE:
> + case RMNET_QUEUE_DISABLE:
> + p = xa_load(&priv->queue_map, mark);
> + if (p && xa_is_value(p)) {
> + txq = xa_to_value(p);
> +
> + q = netdev_get_tx_queue(dev, txq);
> + if (unlikely(!q)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "unsupported queue mapping");
> + return -EINVAL;
> + }
> +
> + if (operation == RMNET_QUEUE_ENABLE)
> + netif_tx_wake_queue(q);
> + else
> + netif_tx_stop_queue(q);
> + } else {
> + NL_SET_ERR_MSG_MOD(extack, "invalid queue mapping");
> + return -EINVAL;
> + }
> + break;
> + default:
> + NL_SET_ERR_MSG_MOD(extack, "unsupported operation");
> + return -EINVAL;
I'm wondering if EOPNOTSUPP is appropriate here.
> + }
> +
> + return 0;
> +}
> +
> static void rmnet_unregister_bridge(struct rmnet_port *port)
> {
> struct net_device *bridge_dev, *real_dev, *rmnet_dev;
> @@ -175,8 +237,24 @@ static int rmnet_newlink(struct net *src_net, struct net_device *dev,
> netdev_dbg(dev, "data format [0x%08X]\n", data_format);
> port->data_format = data_format;
>
> + if (data[IFLA_RMNET_QUEUE]) {
> + struct rmnet_queue_mapping *queue_map;
> +
> + queue_map = nla_data(data[IFLA_RMNET_QUEUE]);
> + if (rmnet_update_queue_map(dev, queue_map->operation,
> + queue_map->txqueue, queue_map->mark,
> + extack))
Should the return value of rmnet_update_queue_map() be stored in err
so that it is also the return value of this function?
> + goto err3;
> +
> + netdev_dbg(dev, "op %02x txq %02x mark %08x\n",
> + queue_map->operation, queue_map->txqueue,
> + queue_map->mark);
> + }
> +
> return 0;
>
> +err3:
> + hlist_del_init_rcu(&ep->hlnode);
Is a call to netdev_upper_dev_unlink() needed here?
> err2:
> unregister_netdevice(dev);
> rmnet_vnd_dellink(mux_id, port, ep);
> @@ -352,6 +430,20 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
> }
> }
>
> + if (data[IFLA_RMNET_QUEUE]) {
> + struct rmnet_queue_mapping *queue_map;
> +
> + queue_map = nla_data(data[IFLA_RMNET_QUEUE]);
> + if (rmnet_update_queue_map(dev, queue_map->operation,
> + queue_map->txqueue, queue_map->mark,
> + extack))
> + return -EINVAL;
I guess that with the current implementation of rmnet_update_queue_map()
it makes no difference, but perhaps it would be better to return
the return value of rmnet_update_queue_map() rather than hard coding
-EINVAL here.
> +
> + netdev_dbg(dev, "op %02x txq %02x mark %08x\n",
> + queue_map->operation, queue_map->txqueue,
> + queue_map->mark);
> + }
> +
> return 0;
> }
>
...
Powered by blists - more mailing lists