| 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: <c6597621-dbb5-4891-8aba-f0596b08e667@davidwei.uk>
Date: Fri, 5 Apr 2024 17:20:05 -0700
From: David Wei <dw@...idwei.uk>
To: Amritha Nambiar <amritha.nambiar@...el.com>, netdev@...r.kernel.org,
kuba@...nel.org, davem@...emloft.net
Cc: edumazet@...gle.com, pabeni@...hat.com, ast@...nel.org, sdf@...gle.com,
lorenzo@...nel.org, tariqt@...dia.com, daniel@...earbox.net,
anthony.l.nguyen@...el.com, lucien.xin@...il.com, hawk@...nel.org,
sridhar.samudrala@...el.com
Subject: Re: [net-next, RFC PATCH 2/5] netdev-genl: Add netlink framework
functions for queue-set NAPI
On 2024-04-05 13:09, Amritha Nambiar wrote:
> Implement the netdev netlink framework functions for associating
> a queue with NAPI ID.
>
> Signed-off-by: Amritha Nambiar <amritha.nambiar@...el.com>
> ---
> include/linux/netdevice.h | 7 +++
> net/core/netdev-genl.c | 117 +++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 108 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0c198620ac93..70df1cec4a60 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1351,6 +1351,10 @@ struct netdev_net_notifier {
> * struct kernel_hwtstamp_config *kernel_config,
> * struct netlink_ext_ack *extack);
> * Change the hardware timestamping parameters for NIC device.
> + *
> + * int (*ndo_queue_set_napi)(struct net_device *dev, u32 q_idx, u32 q_type,
> + * struct napi_struct *napi);
> + * Change the NAPI instance associated with the queue.
> */
> struct net_device_ops {
> int (*ndo_init)(struct net_device *dev);
> @@ -1596,6 +1600,9 @@ struct net_device_ops {
> int (*ndo_hwtstamp_set)(struct net_device *dev,
> struct kernel_hwtstamp_config *kernel_config,
> struct netlink_ext_ack *extack);
> + int (*ndo_queue_set_napi)(struct net_device *dev,
> + u32 q_idx, u32 q_type,
> + struct napi_struct *napi);
> };
>
> /**
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index d5b2e90e5709..6b3d3165d76e 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -288,12 +288,29 @@ int netdev_nl_napi_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> return err;
> }
>
> +/* must be called under rtnl_lock() */
> +static struct napi_struct *
> +napi_get_by_queue(struct net_device *netdev, u32 q_idx, u32 q_type)
> +{
> + struct netdev_rx_queue *rxq;
> + struct netdev_queue *txq;
> +
> + switch (q_type) {
> + case NETDEV_QUEUE_TYPE_RX:
> + rxq = __netif_get_rx_queue(netdev, q_idx);
> + return rxq->napi;
> + case NETDEV_QUEUE_TYPE_TX:
> + txq = netdev_get_tx_queue(netdev, q_idx);
> + return txq->napi;
> + }
> + return NULL;
> +}
> +
> static int
> netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
> u32 q_idx, u32 q_type, const struct genl_info *info)
> {
> - struct netdev_rx_queue *rxq;
> - struct netdev_queue *txq;
> + struct napi_struct *napi;
> void *hdr;
>
> hdr = genlmsg_iput(rsp, info);
> @@ -305,19 +322,9 @@ netdev_nl_queue_fill_one(struct sk_buff *rsp, struct net_device *netdev,
> nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex))
> goto nla_put_failure;
>
> - switch (q_type) {
> - case NETDEV_QUEUE_TYPE_RX:
> - rxq = __netif_get_rx_queue(netdev, q_idx);
> - if (rxq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID,
> - rxq->napi->napi_id))
> - goto nla_put_failure;
> - break;
> - case NETDEV_QUEUE_TYPE_TX:
> - txq = netdev_get_tx_queue(netdev, q_idx);
> - if (txq->napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID,
> - txq->napi->napi_id))
> - goto nla_put_failure;
> - }
> + napi = napi_get_by_queue(netdev, q_idx, q_type);
> + if (napi && nla_put_u32(rsp, NETDEV_A_QUEUE_NAPI_ID, napi->napi_id))
> + goto nla_put_failure;
>
> genlmsg_end(rsp, hdr);
>
> @@ -674,9 +681,87 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
> return err;
> }
>
> +static int
> +netdev_nl_queue_set_napi(struct sk_buff *rsp, struct net_device *netdev,
> + u32 q_idx, u32 q_type, u32 napi_id,
> + const struct genl_info *info)
> +{
> + struct napi_struct *napi, *old_napi;
> + int err;
> +
> + if (!(netdev->flags & IFF_UP))
> + return 0;
Should this be an error code?
> +
> + err = netdev_nl_queue_validate(netdev, q_idx, q_type);
> + if (err)
> + return err;
> +
> + old_napi = napi_get_by_queue(netdev, q_idx, q_type);
> + if (old_napi && old_napi->napi_id == napi_id)
> + return 0;
Same as above, I think this should be an error.
> +
> + napi = napi_by_id(napi_id);
> + if (!napi)
> + return -EINVAL;
> +
> + err = netdev->netdev_ops->ndo_queue_set_napi(netdev, q_idx, q_type, napi);
> +
> + return err;
nit: return ndo_queue_set_napi() would save two lines.
> +}
> +
> int netdev_nl_queue_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
> - return -EOPNOTSUPP;
> + u32 q_id, q_type, ifindex;
nit: q_idx for consistency?
> + struct net_device *netdev;
> + struct sk_buff *rsp;
> + u32 napi_id = 0;
> + int err = 0;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_ID) ||
> + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_TYPE) ||
> + GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX))
> + return -EINVAL;
> +
> + q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_ID]);
> + q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_TYPE]);
> + ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
> +
> + if (info->attrs[NETDEV_A_QUEUE_NAPI_ID]) {
> + napi_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_NAPI_ID]);
> + if (napi_id < MIN_NAPI_ID)
> + return -EINVAL;
> + }
> +
> + rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!rsp)
> + return -ENOMEM;
> +
> + rtnl_lock();
> +
> + netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> + if (netdev) {
> + if (!napi_id)
> + GENL_SET_ERR_MSG(info, "No queue parameters changed\n");
Could this be checked earlier outside of rtnl_lock? I feel like not
setting a napi_id here is EINVAL.
> + else
> + err = netdev_nl_queue_set_napi(rsp, netdev, q_id,
> + q_type, napi_id, info);
> + if (!err)
> + err = netdev_nl_queue_fill_one(rsp, netdev, q_id,
> + q_type, info);
> + } else {
> + err = -ENODEV;
Could be less nesty (completely untested):
if (!netdev) {
err = -ENODEV;
goto err_rtnl_unlock;
}
if (!napi_id) {
GENL_SET_ERR_MSG(info, "No queue parameters changed\n");
goto err_nonapi;
}
err = netdev_nl_queue_set_napi(rsp, netdev, q_id,
q_type, napi_id, info);
if (err)
goto err_rtnl_unlock;
err_nonapi:
err = netdev_nl_queue_fill_one(rsp, netdev, q_id,
q_type, info);
err_rtnl_unlock:
rtnl_unlock();
if (!err)
return genlmsg_reply(rsp, info);
err_free_msg:
nlmsg_free(rsp);
return err;
> + }
> +
> + rtnl_unlock();
> +
> + if (err)
> + goto err_free_msg;
> +
> + return genlmsg_reply(rsp, info);
> +
> +err_free_msg:
> + nlmsg_free(rsp);
> + return err;
> }
>
> static int netdev_genl_netdevice_event(struct notifier_block *nb,
>
Powered by blists - more mailing lists