netdev - Re: [PATCH v4 net-next 06/21] 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: <3f447ef7-3ded-8c1c-fdfa-ec137e11a19a@gmail.com>
Date:   Sun, 21 Feb 2021 13:29:39 +0200
From:   Boris Pismenny <borispismenny@...il.com>
To:     Or Gerlitz <gerlitz.or@...il.com>, David Ahern <dsahern@...il.com>
Cc:     Boris Pismenny <borisp@...lanox.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Saeed Mahameed <saeedm@...dia.com>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>, axboe@...com,
        Keith Busch <kbusch@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Eric Dumazet <edumazet@...gle.com>, smalin@...vell.com,
        Yoray Zack <yorayz@...lanox.com>, yorayz@...dia.com,
        boris.pismenny@...il.com, Ben Ben-Ishay <benishay@...lanox.com>,
        benishay@...dia.com, linux-nvme@...ts.infradead.org,
        Linux Netdev List <netdev@...r.kernel.org>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Or Gerlitz <ogerlitz@...dia.com>
Subject: Re: [PATCH v4 net-next 06/21] nvme-tcp: Add DDP offload control path

On 17/02/2021 15:55, Or Gerlitz wrote:
> On Sun, Feb 14, 2021 at 8:20 PM David Ahern <dsahern@...il.com> wrote:
>> On 2/11/21 2:10 PM, Boris Pismenny wrote:
>>> @@ -223,6 +229,164 @@ static inline size_t nvme_tcp_pdu_last_send(struct nvme_tcp_request *req,
>>>       return nvme_tcp_pdu_data_left(req) <= len;
>>>  }
>>>
>>> +#ifdef CONFIG_TCP_DDP
>>> +
>>> +static bool nvme_tcp_resync_request(struct sock *sk, u32 seq, u32 flags);
>>> +static const struct tcp_ddp_ulp_ops nvme_tcp_ddp_ulp_ops = {
>>> +     .resync_request         = nvme_tcp_resync_request,
>>> +};
>>> +
>>> +static int nvme_tcp_offload_socket(struct nvme_tcp_queue *queue)
>>> +{
>>> +     struct net_device *netdev = queue->ctrl->offloading_netdev;
>>> +     struct nvme_tcp_ddp_config config = {};
>>> +     int ret;
>>> +
>>> +     if (!(netdev->features & NETIF_F_HW_TCP_DDP))
>>
>> If nvme_tcp_offload_limits does not find a dst_entry on the socket then
>> offloading_netdev may not NULL at this point.
> 
> correct :( will look on that
> 

That's only partially true.
If nvme_tcp_offload_limits finds a dst_entry, but then the netdevice
goes down, then the check here will catch it. This is needed because
nvme_tcp_offload_limits doesn't hold a reference! We opted not to grab a
reference on nvme_tcp_offload_limits because it doesn't create a context.


>>> +             queue->ctrl->offloading_netdev = NULL;
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     if (netdev->features & NETIF_F_HW_TCP_DDP &&
>>> +         netdev->tcp_ddp_ops &&
>>> +         netdev->tcp_ddp_ops->tcp_ddp_limits)
>>> +             ret = netdev->tcp_ddp_ops->tcp_ddp_limits(netdev, &limits);
>>> +     else
>>> +             ret = -EOPNOTSUPP;
>>> +
>>> +     if (!ret) {
>>> +             queue->ctrl->offloading_netdev = netdev;
>>
>> you save a reference to the netdev here, but then release the refcnt
>> below. That device could be deleted between this point in time and the
>> initialization of all queues.
> 

That's true, and this is why we repeat the checks there.

We avoid holding the reference here because there is no obvious
complementary release point for nvme_tcp_offload_limits and there is no
hardware context created here, so there is no real need to hold it at
this stage.