netdev - Re: [PATCH v20 02/20] netlink: add new family to manage ULP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 27 Nov 2023 15:17:50 +0200
From: Aurelien Aptel <aaptel@...dia.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: linux-nvme@...ts.infradead.org, netdev@...r.kernel.org,
 sagi@...mberg.me, hch@....de, kbusch@...nel.org, axboe@...com,
 chaitanyak@...dia.com, davem@...emloft.net, kuba@...nel.org,
 aurelien.aptel@...il.com, smalin@...dia.com, malin1024@...il.com,
 ogerlitz@...dia.com, yorayz@...dia.com, borisp@...dia.com,
 galshalom@...dia.com, mgurtovoy@...dia.com, edumazet@...gle.com,
 pabeni@...hat.com
Subject: Re: [PATCH v20 02/20] netlink: add new family to manage ULP_DDP
 enablement and stats

Jiri Pirko <jiri@...nulli.us> writes:
> Overall looks good to me, couple of nits below, take it or leave it.

Glad to hear. We will send v21 with the changes below.
Hopefully we can get a RB or Ack on this patch.

>>+
>>+      ctx->ifindex = nla_get_u32(info->attrs[ULP_DDP_A_CAPS_IFINDEX]);
>
> You don't need to store ifindex. You have ctx->dev->ifindex with the
> same value.

Ok, we will remove it from the context.

>>+      switch (cmd) {
>>+      case ULP_DDP_CMD_CAPS_GET:
>>+      case ULP_DDP_CMD_CAPS_SET:
>>+      case ULP_DDP_CMD_CAPS_SET_NTF:
>
> Hmm, you never call ulp_ddp_prepare_context() with
> ULP_DDP_CMD_CAPS_SET_NTF. Perhaps just warn if this case is hit. IDK.

While that is true it also reflects what the payload is and it mirrors
ulp_ddp_reply_size(). We will keep it as is.

>>+static int ulp_ddp_write_reply(struct sk_buff *rsp,
>>+                             struct ulp_ddp_reply_context *ctx,
>>+                             int cmd,
>>+                             const struct genl_info *info)
>>+{
>>+      ctx->hdr = genlmsg_iput(rsp, info);
>
> Interesting, you use "hdr" just in this fuction. Why isn't it a local
> variable?

Good point, we will make it a local variable.

>>+      ret = ulp_ddp_write_reply(rsp, ctx, ULP_DDP_CMD_CAPS_GET, info);
>>+      if (ret < 0)
>
> You mix "if (ret)" and "if (ret < 0)" in this code, I don't see any of
> the functions return positive value on success, so you can make it
> consistent to "if (ret)".

Ok, we will change the checks to "if (ret)" and change
ulp_ddp_apply_bits() return the notify flag via an input parameter.

>>+
>>+      if (ulp_ddp_write_reply(ntf, ctx, ULP_DDP_CMD_CAPS_SET_NTF, &info)) {
>
> Always use "ret" variable to store return value before you check it.

Ok.

>>+
>>+      rsp = genlmsg_new(ulp_ddp_reply_size(ULP_DDP_CMD_STATS_GET), GFP_KERNEL);
>>+      if (!rsp)
>>+              return -EMSGSIZE;
>>+
>>+      wanted = nla_get_u64(info->attrs[ULP_DDP_A_CAPS_WANTED]);
>
> nla_get_uint()
>
>
>>+      wanted_mask = nla_get_u64(info->attrs[ULP_DDP_A_CAPS_WANTED_MASK]);
>
> nla_get_uint()
>

Ok.

>
>>+
>>+      ret = ulp_ddp_apply_bits(ctx, &wanted, &wanted_mask, info);
>
>         ret = ulp_ddp_apply_bits(ctx, &wanted, &wanted_mask, info,
>                                  &notify);
>
> Would be a bit cleaner to read perhaps.

True, we will change it to that.

Thanks