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