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: <fe26f9b6-ff3d-441d-887d-9f65d44f06d0@intel.com> Date: Wed, 11 Oct 2023 16:54:00 -0700 From: "Nambiar, Amritha" <amritha.nambiar@...el.com> To: Jakub Kicinski <kuba@...nel.org> 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 10/10/2023 7:25 PM, Jakub Kicinski wrote: > 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 || > Okay, will fix in v5. >> + 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? For this, should I be using "checks: max:" as an attribute property for the 'queue-id' attribute in the yaml. If so, not sure how I can give a custom value (not hard-coded) for max. Or should I include a pre-doit hook. > >> + } >> +} > >> 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 || Will fix in v5. > >> + 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. Will fix. > >> + 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 = Will fix. > >> + 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? > Makes sense. Will fix in v5. >> + 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 Will fix. > >> + *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 Will fix. > >> + if (err < 0) >> + break; >> + if (!err) { > > it only returns 0 or negative errno, doesn't it? > Will fix in v5. >> + 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