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