netdev - Re: [PATCH v20 05/20] nvme-tcp: Add DDP offload control path

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]

Message-ID: <97e6efa4-8d70-98e1-f5af-1d34672c2e2b@nvidia.com>
Date: Mon, 11 Dec 2023 19:29:09 +0200
From: Max Gurtovoy <mgurtovoy@...dia.com>
To: Aurelien Aptel <aaptel@...dia.com>, Sagi Grimberg <sagi@...mberg.me>,
 linux-nvme@...ts.infradead.org, netdev@...r.kernel.org, hch@....de,
 kbusch@...nel.org, axboe@...com, chaitanyak@...dia.com, davem@...emloft.net,
 kuba@...nel.org
Cc: Boris Pismenny <borisp@...dia.com>, aurelien.aptel@...il.com,
 smalin@...dia.com, malin1024@...il.com, ogerlitz@...dia.com,
 yorayz@...dia.com, galshalom@...dia.com, brauner@...nel.org
Subject: Re: [PATCH v20 05/20] nvme-tcp: Add DDP offload control path

Hi Sagi,

On 29/11/2023 15:52, Aurelien Aptel wrote:
> Hi Sagi,
> 
> Sagi Grimberg <sagi@...mberg.me> writes:
>>> +     ok = ulp_ddp_query_limits(netdev, &ctrl->ddp_limits,
>>> +                               ULP_DDP_NVME, ULP_DDP_CAP_NVME_TCP,
>>> +                               ctrl->ctrl.opts->tls);
>>> +     if (!ok) {
>>
>> please use a normal name (ret).
> 
> Ok, we will rename to ret and make ulp_ddp_query_limits() return int 0
> on success to be consistent with the name.
> 
>> Plus, its strange that a query function receives a feature and returns
>> true/false based on this. The query should return the limits, and the
>> caller should look at the limits and see if it is appropriately
>> supported.
> 
> We are not sure how to proceed as this seems to conflict with what you
> suggested in v12 [1] about hiding the details of checking supports in
> the API. Limits just dictate some constants the nvme-layer should use
> once we know it is supported.
> 
> We can rename ulp_ddp_query_limits() to ulp_ddp_check_support(). This
> function checks the support of the specified offload capability and also
> returns the limitations of it.
> 
> Alternatively, we can split it in 2 API functions (check_support
> and query_limits).
> 
> Let us know what you prefer.
> 
> Thanks
> 
> 1: https://lkml.kernel.org/netdev/bc5cd2a7-efc4-e4df-cae5-5c527dd704a6@grimberg.me/

We would like to submit another version by the end of this week and 
hopefully progress with this series.
To address your comment, I was thinking about something like:

+	if (!ulp_ddp_is_cap_active(netdev, ULP_DDP_CAP_NVME_TCP))
+		goto err;
+
+	ret = ulp_ddp_get_limits(netdev, &ctrl->ddp_limits, ULP_DDP_NVME);
+	if (ret)
+		goto err;
+
+	if (ctrl->ctrl.opts->tls && !ctrl->ddp_limits.tls)
+		goto err;

I would like to remind you that the community didn't want to add ulp_ddp 
caps and limits context to the netdev structure (as we have for example 
in the block layer q) and preferred the design of ops/cbs.