| 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: <20231010192555.3126ca42@kernel.org>
Date: Tue, 10 Oct 2023 19:25:55 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Amritha Nambiar <amritha.nambiar@...el.com>
Cc: netdev@...r.kernel.org, sridhar.samudrala@...el.com
Subject: Re: [net-next PATCH v4 04/10] netdev-genl: Add netlink framework
functions for queue
On Fri, 06 Oct 2023 02:14:59 -0700 Amritha Nambiar wrote:
> +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;
> + void *hdr;
> +
> + hdr = genlmsg_iput(rsp, info);
> + if (!hdr)
> + return -EMSGSIZE;
> +
> + if (nla_put_u32(rsp, NETDEV_A_QUEUE_QUEUE_ID, q_idx))
> + goto nla_put_failure;
> +
> + if (nla_put_u32(rsp, NETDEV_A_QUEUE_QUEUE_TYPE, q_type))
> + goto nla_put_failure;
> +
> + if (nla_put_u32(rsp, NETDEV_A_QUEUE_IFINDEX, netdev->ifindex))
> + goto nla_put_failure;
You can combine these ifs in a single one using ||
> + 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,
> +static int netdev_nl_queue_validate(struct net_device *netdev, u32 q_id,
> + u32 q_type)
> +{
> + switch (q_type) {
> + case NETDEV_QUEUE_TYPE_RX:
> + if (q_id >= netdev->real_num_rx_queues)
> + return -EINVAL;
> + return 0;
> + case NETDEV_QUEUE_TYPE_TX:
> + if (q_id >= netdev->real_num_tx_queues)
> + return -EINVAL;
> + return 0;
> + default:
> + return -EOPNOTSUPP;
Doesn't the netlink policy prevent this already?
> + }
> +}
> int netdev_nl_queue_get_doit(struct sk_buff *skb, struct genl_info *info)
> {
> - return -EOPNOTSUPP;
> + u32 q_id, q_type, ifindex;
> + struct net_device *netdev;
> + struct sk_buff *rsp;
> + int err;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_QUEUE_ID))
> + return -EINVAL;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_QUEUE_TYPE))
> + return -EINVAL;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_QUEUE_IFINDEX))
> + return -EINVAL;
You can combine these checks in a single if using ||
> + q_id = nla_get_u32(info->attrs[NETDEV_A_QUEUE_QUEUE_ID]);
> +
> + q_type = nla_get_u32(info->attrs[NETDEV_A_QUEUE_QUEUE_TYPE]);
> +
> + ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
No need for the empty lines between these.
> + 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)
> + err = netdev_nl_queue_fill(rsp, netdev, q_id, q_type, info);
double space after =
> + else
> + err = -ENODEV;
> +
> + rtnl_unlock();
> +
> + if (err)
> + goto err_free_msg;
> +
> + return genlmsg_reply(rsp, info);
> +
> +err_free_msg:
> + nlmsg_free(rsp);
> + return err;
> +}
> +
> +static int
> +netdev_nl_queue_dump_one(struct net_device *netdev, struct sk_buff *rsp,
> + const struct genl_info *info, unsigned int *start_rx,
> + unsigned int *start_tx)
> +{
Hm. Not sure why you don't operate directly on ctx here.
Why pass the indexes by pointer individually?
> + int err = 0;
> + int i;
> +
> + for (i = *start_rx; i < netdev->real_num_rx_queues;) {
> + err = netdev_nl_queue_fill_one(rsp, netdev, i,
> + NETDEV_QUEUE_TYPE_RX, info);
> + if (err)
> + goto out_err;
return, no need to goto if all it does is returns
> + *start_rx = i++;
> + }
> + for (i = *start_tx; i < netdev->real_num_tx_queues;) {
> + err = netdev_nl_queue_fill_one(rsp, netdev, i,
> + NETDEV_QUEUE_TYPE_TX, info);
> + if (err)
> + goto out_err;
> + *start_tx = i++;
> + }
> +out_err:
> + return err;
> }
>
> int netdev_nl_queue_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> {
> - return -EOPNOTSUPP;
> + struct netdev_nl_dump_ctx *ctx = netdev_dump_ctx(cb);
> + const struct genl_info *info = genl_info_dump(cb);
> + struct net *net = sock_net(skb->sk);
> + unsigned int rxq_idx = ctx->rxq_idx;
> + unsigned int txq_idx = ctx->txq_idx;
> + struct net_device *netdev;
> + u32 ifindex = 0;
> + int err = 0;
> +
> + if (info->attrs[NETDEV_A_QUEUE_IFINDEX])
> + ifindex = nla_get_u32(info->attrs[NETDEV_A_QUEUE_IFINDEX]);
> +
> + rtnl_lock();
> + if (ifindex) {
> + netdev = __dev_get_by_index(net, ifindex);
> + if (netdev)
> + err = netdev_nl_queue_dump_one(netdev, skb, info,
> + &rxq_idx, &txq_idx);
> + else
> + err = -ENODEV;
> + } else {
> + for_each_netdev_dump(net, netdev, ctx->ifindex) {
> + err = netdev_nl_queue_dump_one(netdev, skb, info,
> + &rxq_idx, &txq_idx);
> +
unnecessary new line
> + if (err < 0)
> + break;
> + if (!err) {
it only returns 0 or negative errno, doesn't it?
> + rxq_idx = 0;
> + txq_idx = 0;
> + }
> + }
> + }
> + rtnl_unlock();
> +
> + if (err != -EMSGSIZE)
> + return err;
> +
> + ctx->rxq_idx = rxq_idx;
> + ctx->txq_idx = txq_idx;
> + return skb->len;
> }
>
> static int netdev_genl_netdevice_event(struct notifier_block *nb,
>
Powered by blists - more mailing lists