| 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: <2dd10b2f-df00-e21c-7886-93f41a987040@gmail.com>
Date: Sun, 14 Feb 2021 11:16:30 -0700
From: David Ahern <dsahern@...il.com>
To: Boris Pismenny <borisp@...lanox.com>, kuba@...nel.org,
davem@...emloft.net, saeedm@...dia.com, hch@....de,
sagi@...mberg.me, axboe@...com, kbusch@...nel.org,
viro@...iv.linux.org.uk, edumazet@...gle.com, smalin@...vell.com
Cc: boris.pismenny@...il.com, linux-nvme@...ts.infradead.org,
netdev@...r.kernel.org, benishay@...dia.com, ogerlitz@...dia.com,
yorayz@...dia.com, Ben Ben-Ishay <benishay@...lanox.com>,
Or Gerlitz <ogerlitz@...lanox.com>,
Yoray Zack <yorayz@...lanox.com>
Subject: Re: [PATCH v4 net-next 06/21] nvme-tcp: Add DDP offload control path
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.
> + return -EOPNOTSUPP;
> +
> + config.cfg.type = TCP_DDP_NVME;
> + config.pfv = NVME_TCP_PFV_1_0;
> + config.cpda = 0;
> + config.dgst = queue->hdr_digest ?
> + NVME_TCP_HDR_DIGEST_ENABLE : 0;
> + config.dgst |= queue->data_digest ?
> + NVME_TCP_DATA_DIGEST_ENABLE : 0;
> + config.queue_size = queue->queue_size;
> + config.queue_id = nvme_tcp_queue_id(queue);
> + config.io_cpu = queue->io_cpu;
> +
> + dev_hold(netdev); /* put by unoffload_socket */
> + ret = netdev->tcp_ddp_ops->tcp_ddp_sk_add(netdev,
> + queue->sock->sk,
> + &config.cfg);
> + if (ret) {
> + dev_put(netdev);
> + return ret;
> + }
> +
> + inet_csk(queue->sock->sk)->icsk_ulp_ddp_ops = &nvme_tcp_ddp_ulp_ops;
> + if (netdev->features & NETIF_F_HW_TCP_DDP)
> + set_bit(NVME_TCP_Q_OFF_DDP, &queue->flags);
> +
> + return ret;
> +}
> +
> +static void nvme_tcp_unoffload_socket(struct nvme_tcp_queue *queue)
> +{
> + struct net_device *netdev = queue->ctrl->offloading_netdev;
> +
> + if (!netdev) {
> + dev_info_ratelimited(queue->ctrl->ctrl.device, "netdev not found\n");
you are already logged in nvme_tcp_offload_limits that
get_netdev_for_sock returned NULL; no need to do it again.
> + return;
> + }
> +
> + netdev->tcp_ddp_ops->tcp_ddp_sk_del(netdev, queue->sock->sk);
> +
> + inet_csk(queue->sock->sk)->icsk_ulp_ddp_ops = NULL;
> + dev_put(netdev); /* held by offload_socket */
> +}
> +
> +static int nvme_tcp_offload_limits(struct nvme_tcp_queue *queue)
> +{
> + struct net_device *netdev = get_netdev_for_sock(queue->sock->sk, true);
> + struct tcp_ddp_limits limits;
> + int ret = 0;
> +
> + if (!netdev) {
> + dev_info_ratelimited(queue->ctrl->ctrl.device, "netdev not found\n");
This should be more informative.
> + 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.
> + dev_dbg_ratelimited(queue->ctrl->ctrl.device,
> + "netdev %s offload limits: max_ddp_sgl_len %d\n",
> + netdev->name, limits.max_ddp_sgl_len);
> + queue->ctrl->ctrl.max_segments = limits.max_ddp_sgl_len;
> + queue->ctrl->ctrl.max_hw_sectors =
> + limits.max_ddp_sgl_len << (ilog2(SZ_4K) - 9);
> + } else {
> + queue->ctrl->offloading_netdev = NULL;
> + }
> +
> + /* release the device as no offload context is established yet. */
> + dev_put(netdev);
> +
> + return ret;
> +}
> +
> +static void nvme_tcp_resync_response(struct nvme_tcp_queue *queue,
> + struct sk_buff *skb, unsigned int offset)
> +{
> + u64 pdu_seq = TCP_SKB_CB(skb)->seq + offset - queue->pdu_offset;
> + struct net_device *netdev = queue->ctrl->offloading_netdev;
> + u64 resync_val;
> + u32 resync_seq;
> +
> + resync_val = atomic64_read(&queue->resync_req);
> + /* Lower 32 bit flags. Check validity of the request */
> + if ((resync_val & TCP_DDP_RESYNC_REQ) == 0)
> + return;
> +
> + /* Obtain and check requested sequence number: is this PDU header before the request? */
> + resync_seq = resync_val >> 32;
> + if (before(pdu_seq, resync_seq))
> + return;
> +
> + if (unlikely(!netdev)) {
> + pr_info_ratelimited("%s: netdev not found\n", __func__);
can't happen right? you get here because NVME_TCP_Q_OFF_DDP is set and
it is only set if offloading_netdev is set and the device supports offload.
> + return;
> + }
> +
> + /**
> + * The atomic operation gurarantees that we don't miss any NIC driver
> + * resync requests submitted after the above checks.
> + */
> + if (atomic64_cmpxchg(&queue->resync_req, resync_val,
> + resync_val & ~TCP_DDP_RESYNC_REQ))
> + netdev->tcp_ddp_ops->tcp_ddp_resync(netdev, queue->sock->sk, pdu_seq);
> +}
> +
Powered by blists - more mailing lists