From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9F942185D for ; Tue, 15 Aug 2023 07:01:29 +0000 (UTC) Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com [209.85.218.51]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 08D451FC9 for ; Tue, 15 Aug 2023 00:01:27 -0700 (PDT) Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-99bded9d93dso110880066b.1 for ; Tue, 15 Aug 2023 00:01:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692082885; x=1692687685; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=kW5rUcBUNkKENpM5TxSALtfybP7YaSOa8PTQFN1lGLw=; b=FyjdTkLDun48Phzv9bHwFXL+hB/mI11llfXEmzOs9zWnHvGYyZLeYxbgae2K8QsEn1 bpMB2LXrdWi4jmxg+NuLqieaotYfWK8qbxFpthfyHgYnIIPcqaC/gp7QLKcfvZw5ryfF 65Z1HXfVrMzg0c54A5iYr5md1/wvyoPuxeHm9kd8xuqLb505lIff4bB6RcxswqfF7CeC cM2HpJU576v5hb+BxUweEk1TZXpMoGvQ/TIFszcND4Y9Zb3cDEohHl5h6wyJx4V2EW3X wST/E0GB/4NDPP6RpHx6fgTa6KnmoNlLBM8JLhz40xHfxp5vTlK4I0CDP1UqAW4dH+3a e9kw== X-Gm-Message-State: AOJu0YwrezznWjlNs7ENOdmh2gIb0crktxpwUVLBjF/qXKnEZCOkjmjk pgEosZvSQYOfHE7mvI9n8Eo= X-Google-Smtp-Source: AGHT+IHbtKsmqwWTlucfMvubexMctgEqw9CNRShuZMwyRY9akQ145vMpCRn2GiEUzehunpWfLc9vTA== X-Received: by 2002:a17:906:19:b0:993:d0e1:f308 with SMTP id 25-20020a170906001900b00993d0e1f308mr8286680eja.2.1692082885262; Tue, 15 Aug 2023 00:01:25 -0700 (PDT) Received: from [192.168.64.157] (bzq-219-42-90.isdn.bezeqint.net. [62.219.42.90]) by smtp.gmail.com with ESMTPSA id jo19-20020a170906f6d300b0099bcd1fa5b0sm6613092ejb.192.2023.08.15.00.01.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 15 Aug 2023 00:01:24 -0700 (PDT) Message-ID: Date: Tue, 15 Aug 2023 10:01:21 +0300 Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH 15/17] nvmet-tcp: enable TLS handshake upcall Content-Language: en-US To: Hannes Reinecke , Christoph Hellwig Cc: Keith Busch , linux-nvme@lists.infradead.org, Jakub Kicinski , Eric Dumazet , Paolo Abeni , netdev@vger.kernel.org References: <20230814111943.68325-1-hare@suse.de> <20230814111943.68325-16-hare@suse.de> From: Sagi Grimberg In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, NICE_REPLY_A,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net >>>>> @@ -1864,6 +1877,14 @@ static struct config_group >>>>> *nvmet_ports_make(struct config_group *group, >>>>>           return ERR_PTR(-ENOMEM); >>>>>       } >>>>> +    if (nvme_keyring_id()) { >>>>> +        port->keyring = key_lookup(nvme_keyring_id()); >>>>> +        if (IS_ERR(port->keyring)) { >>>>> +            pr_warn("NVMe keyring not available, disabling TLS\n"); >>>>> +            port->keyring = NULL; >>>> >>>> why setting this to NULL? >>>> >>> It's check when changing TSAS; we can only enable TLS if the nvme >>> keyring is available. >> >> ok >> >>> >>>>> +        } >>>>> +    } >>>>> + >>>>>       for (i = 1; i <= NVMET_MAX_ANAGRPS; i++) { >>>>>           if (i == NVMET_DEFAULT_ANA_GRPID) >>>>>               port->ana_state[1] = NVME_ANA_OPTIMIZED; >>>>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h >>>>> index 8cfd60f3b564..7f9ae53c1df5 100644 >>>>> --- a/drivers/nvme/target/nvmet.h >>>>> +++ b/drivers/nvme/target/nvmet.h >>>>> @@ -158,6 +158,7 @@ struct nvmet_port { >>>>>       struct config_group        ana_groups_group; >>>>>       struct nvmet_ana_group        ana_default_group; >>>>>       enum nvme_ana_state        *ana_state; >>>>> +    struct key            *keyring; >>>>>       void                *priv; >>>>>       bool                enabled; >>>>>       int                inline_data_size; >>>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c >>>>> index f19ea9d923fd..77fa339008e1 100644 >>>>> --- a/drivers/nvme/target/tcp.c >>>>> +++ b/drivers/nvme/target/tcp.c >>>>> @@ -8,9 +8,13 @@ >>>>>   #include >>>>>   #include >>>>>   #include >>>>> +#include >>>>>   #include >>>>> +#include >>>>>   #include >>>>>   #include >>>>> +#include >>>>> +#include >>>>>   #include >>>>>   #include >>>>>   #include >>>>> @@ -66,6 +70,16 @@ device_param_cb(idle_poll_period_usecs, >>>>> &set_param_ops, >>>>>   MODULE_PARM_DESC(idle_poll_period_usecs, >>>>>           "nvmet tcp io_work poll till idle time period in usecs: >>>>> Default 0"); >>>>> +#ifdef CONFIG_NVME_TARGET_TCP_TLS >>>>> +/* >>>>> + * TLS handshake timeout >>>>> + */ >>>>> +static int tls_handshake_timeout = 10; >>>>> +module_param(tls_handshake_timeout, int, 0644); >>>>> +MODULE_PARM_DESC(tls_handshake_timeout, >>>>> +         "nvme TLS handshake timeout in seconds (default 10)"); >>>>> +#endif >>>>> + >>>>>   #define NVMET_TCP_RECV_BUDGET        8 >>>>>   #define NVMET_TCP_SEND_BUDGET        8 >>>>>   #define NVMET_TCP_IO_WORK_BUDGET    64 >>>>> @@ -122,11 +136,13 @@ struct nvmet_tcp_cmd { >>>>>   enum nvmet_tcp_queue_state { >>>>>       NVMET_TCP_Q_CONNECTING, >>>>> +    NVMET_TCP_Q_TLS_HANDSHAKE, >>>>>       NVMET_TCP_Q_LIVE, >>>>>       NVMET_TCP_Q_DISCONNECTING, >>>>>   }; >>>>>   struct nvmet_tcp_queue { >>>>> +    struct kref        kref; >>>> >>>> Why is kref the first member of the struct? >>>> >>> Habit. >>> I don't mind where it'll end up. >> >> Move it to the back together with the tls section. >> >>> >>>>>       struct socket        *sock; >>>>>       struct nvmet_tcp_port    *port; >>>>>       struct work_struct    io_work; >>>>> @@ -155,6 +171,10 @@ struct nvmet_tcp_queue { >>>>>       struct ahash_request    *snd_hash; >>>>>       struct ahash_request    *rcv_hash; >>>>> +    /* TLS state */ >>>>> +    key_serial_t        tls_pskid; >>>>> +    struct delayed_work    tls_handshake_work; >>>>> + >>>>>       unsigned long           poll_end; >>>>>       spinlock_t        state_lock; >>>>> @@ -1283,12 +1303,21 @@ static int nvmet_tcp_try_recv(struct >>>>> nvmet_tcp_queue *queue, >>>>>       return ret; >>>>>   } >>>>> +static void nvmet_tcp_release_queue(struct kref *kref) >>>>> +{ >>>>> +    struct nvmet_tcp_queue *queue = >>>>> +        container_of(kref, struct nvmet_tcp_queue, kref); >>>>> + >>>>> +    WARN_ON(queue->state != NVMET_TCP_Q_DISCONNECTING); >>>>> +    queue_work(nvmet_wq, &queue->release_work); >>>>> +} >>>>> + >>>>>   static void nvmet_tcp_schedule_release_queue(struct >>>>> nvmet_tcp_queue *queue) >>>>>   { >>>>>       spin_lock_bh(&queue->state_lock); >>>>>       if (queue->state != NVMET_TCP_Q_DISCONNECTING) { >>>>>           queue->state = NVMET_TCP_Q_DISCONNECTING; >>>>> -        queue_work(nvmet_wq, &queue->release_work); >>>>> +        kref_put(&queue->kref, nvmet_tcp_release_queue); >>>>>       } >>>>>       spin_unlock_bh(&queue->state_lock); >>>>>   } >>>>> @@ -1485,6 +1514,8 @@ static void >>>>> nvmet_tcp_release_queue_work(struct work_struct *w) >>>>>       mutex_unlock(&nvmet_tcp_queue_mutex); >>>>>       nvmet_tcp_restore_socket_callbacks(queue); >>>>> +    tls_handshake_cancel(queue->sock->sk); >>>>> +    cancel_delayed_work_sync(&queue->tls_handshake_work); >>>> >>>> We should call it tls_handshake_tmo_work or something to make it >>>> clear it is a timeout work. >>>> >>> Okay. >>> >>>>>       cancel_work_sync(&queue->io_work); >>>>>       /* stop accepting incoming data */ >>>>>       queue->rcv_state = NVMET_TCP_RECV_ERR; >>>>> @@ -1512,8 +1543,13 @@ static void nvmet_tcp_data_ready(struct sock >>>>> *sk) >>>>>       read_lock_bh(&sk->sk_callback_lock); >>>>>       queue = sk->sk_user_data; >>>>> -    if (likely(queue)) >>>>> -        queue_work_on(queue_cpu(queue), nvmet_tcp_wq, >>>>> &queue->io_work); >>>>> +    if (likely(queue)) { >>>>> +        if (queue->data_ready) >>>>> +            queue->data_ready(sk); >>>>> +        if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) >>>>> +            queue_work_on(queue_cpu(queue), nvmet_tcp_wq, >>>>> +                      &queue->io_work); >>>>> +    } >>>>>       read_unlock_bh(&sk->sk_callback_lock); >>>>>   } >>>>> @@ -1621,6 +1657,83 @@ static int nvmet_tcp_set_queue_sock(struct >>>>> nvmet_tcp_queue *queue) >>>>>       return ret; >>>>>   } >>>>> +#ifdef CONFIG_NVME_TARGET_TCP_TLS >>>>> +static void nvmet_tcp_tls_handshake_done(void *data, int status, >>>>> +                     key_serial_t peerid) >>>>> +{ >>>>> +    struct nvmet_tcp_queue *queue = data; >>>>> + >>>>> +    pr_debug("queue %d: TLS handshake done, key %x, status %d\n", >>>>> +         queue->idx, peerid, status); >>>>> +    spin_lock_bh(&queue->state_lock); >>>>> +    if (queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) { >>>> >>>> Is this even possible? >>>> >>> I guess it can happen when the socket closes during handshake; the >>> daemon might still be sending a 'done' event but >>> nvmet_tcp_schedule_release_queue() has been called. >> >> Umm, if the socket closes during the handshake then the state >> is NVMET_TCP_Q_TLS_HANDSHAKE. >> > But there's a race window between setting it to > NVMET_TCP_Q_DISCONNECTING and calling tls_handshake_cancel(). > >> p.s. you call handshake cancel in the release flow so you should be >> fenced properly no? > Not really. But I'll check if I can fix it up. The teardown handling feels complicated to me. How are you testing it btw?