Re: [PATCH v13 net-next 01/12] tls: support for Inline tls record

[Stefano Brivio]

On Tue, 27 Mar 2018 23:06:30 +0530
Atul Gupta  wrote:

> +static struct tls_context *create_ctx(struct sock *sk)
> +{
> + struct inet_connection_sock *icsk = inet_csk(sk);
> + struct tls_context *ctx;
> +
> + /* allocate tls context */
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return NULL;
> +
> + icsk->icsk_ulp_data = ctx;
> + return ctx;
> +}
>
> [...]
>
>  static int tls_init(struct sock *sk)
>  {
>   int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
> - struct inet_connection_sock *icsk = inet_csk(sk);
>   struct tls_context *ctx;
>   int rc = 0;
>  
> + if (tls_hw_prot(sk))
> + goto out;
> +
>   /* The TLS ulp is currently supported only for TCP sockets
>* in ESTABLISHED state.
>* Supporting sockets in LISTEN state will require us
> @@ -530,12 +624,11 @@ static int tls_init(struct sock *sk)
>   return -ENOTSUPP;
>  
>   /* allocate tls context */
> - ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + ctx = create_ctx(sk);
>   if (!ctx) {
>   rc = -ENOMEM;
>   goto out;
>   }
> - icsk->icsk_ulp_data = ctx;

Why are you changing this?

This is now equivalent to the original implementation, except that you
are "hiding" the assignment of icsk->icsk_ulp_data into a function named
"create_ctx".

Please also note that you are duplicating the "allocate tls context"
comment.

-- 
Stefano

Re: [PATCH v13 net-next 07/12] crypto: chtls - Program the TLS session Key

[Stefano Brivio]

On Tue, 27 Mar 2018 23:06:36 +0530
Atul Gupta  wrote:

> +static void __set_tcb_field(struct sock *sk, struct sk_buff *skb, u16 word,
> + u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);

Please use reverse christmas tree style for variable declarations. If
needed, do the assignments later on.

> [...]
>
> +static int chtls_set_tcb_field(struct sock *sk, u16 word, u64 mask, u64 val)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + struct sk_buff *skb;
> + int ret;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> + unsigned int credits_needed = DIV_ROUND_UP(wrlen, 16);

Same here.

> [...]
>
> +static int get_new_keyid(struct chtls_sock *csk, u32 optname)
> +{
> + struct chtls_hws *hws = >tlshws;
> + struct net_device *dev = csk->egress_dev;
> + struct adapter *adap = netdev2adap(dev);
> + struct chtls_dev *cdev = csk->cdev;
> + int keyid;

Same here.

> +
> + spin_lock_bh(>kmap.lock);
> + keyid = find_first_zero_bit(cdev->kmap.addr, cdev->kmap.size);
> + if (keyid < cdev->kmap.size) {
> + __set_bit(keyid, cdev->kmap.addr);
> + if (optname == TLS_RX)
> + hws->rxkey = keyid;
> + else
> + hws->txkey = keyid;
> + atomic_inc(>chcr_stats.tls_key);
> + } else {
> + keyid = -1;
> + }
> + spin_unlock_bh(>kmap.lock);
> + return keyid;
> +}
> +
> +void free_tls_keyid(struct sock *sk)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct net_device *dev = csk->egress_dev;
> + struct adapter *adap = netdev2adap(dev);
> + struct chtls_dev *cdev = csk->cdev;
> + struct chtls_hws *hws = >tlshws;

Same here.

> [...]
>
> +static int chtls_key_info(struct chtls_sock *csk,
> +   struct _key_ctx *kctx,
> +   u32 keylen, u32 optname)
> +{
> + unsigned char key[CHCR_KEYCTX_CIPHER_KEY_SIZE_256];
> + struct crypto_cipher *cipher;
> + struct tls12_crypto_info_aes_gcm_128 *gcm_ctx =
> + (struct tls12_crypto_info_aes_gcm_128 *)
> + >tlshws.crypto_info;
> + unsigned char ghash_h[AEAD_H_SIZE];
> + int ck_size, key_ctx_size;
> + int ret;

Same here.

> [...]
>
> +int chtls_setkey(struct chtls_sock *csk, u32 keylen, u32 optname)
> +{
> + struct chtls_dev *cdev = csk->cdev;
> + struct sock *sk = csk->sk;
> + struct tls_key_req *kwr;
> + struct _key_ctx *kctx;
> + struct sk_buff *skb;
> + int wrlen, klen, len;
> + int keyid;
> + int kaddr;
> + int ret = 0;

Same here.

> [...]

-- 
Stefano

Re: [PATCH v13 net-next 09/12] crypto: chtls - Inline TLS record Tx

[Stefano Brivio]

On Tue, 27 Mar 2018 23:06:38 +0530
Atul Gupta  wrote:

> +static u8 tcp_state_to_flowc_state(u8 state)
> +{
> + u8 ret = FW_FLOWC_MNEM_TCPSTATE_ESTABLISHED;
> +
> + switch (state) {
> + case TCP_ESTABLISHED:
> + ret = FW_FLOWC_MNEM_TCPSTATE_ESTABLISHED;
> + break;
> + case TCP_CLOSE_WAIT:
> + ret = FW_FLOWC_MNEM_TCPSTATE_CLOSEWAIT;
> + break;
> + case TCP_FIN_WAIT1:
> + ret = FW_FLOWC_MNEM_TCPSTATE_FINWAIT1;
> + break;
> + case TCP_CLOSING:
> + ret = FW_FLOWC_MNEM_TCPSTATE_CLOSING;
> + break;
> + case TCP_LAST_ACK:
> + ret = FW_FLOWC_MNEM_TCPSTATE_LASTACK;
> + break;
> + case TCP_FIN_WAIT2:
> + ret = FW_FLOWC_MNEM_TCPSTATE_FINWAIT2;
> + break;

Can't you just return those values right away instead?

> [...]
>
> +static u64 tlstx_seq_number(struct chtls_hws *hws)
> +{
> + return hws->tx_seq_no++;
> +}

The name of this function, as I also had commented earlier, is
misleading, because you are also incrementing the sequence number.

> [...]
>
> +static void mark_urg(struct tcp_sock *tp, int flags,
> +  struct sk_buff *skb)
> +{
> + if (unlikely(flags & MSG_OOB)) {
> + tp->snd_up = tp->write_seq;
> + ULP_SKB_CB(skb)->flags = ULPCB_FLAG_URG |
> +  ULPCB_FLAG_BARRIER |
> +  ULPCB_FLAG_NO_APPEND |
> +  ULPCB_FLAG_NEED_HDR;

Is this indentation the result of a previous 'if' clause which is now
gone?

> [...]
>
> +/*
> + * Returns true if a TCP socket is corked.
> + */
> +static int corked(const struct tcp_sock *tp, int flags)
> +{
> + return (flags & MSG_MORE) | (tp->nonagle & TCP_NAGLE_CORK);

I guess you meant || here. Shouldn't this be a bool?

> +}
> +
> +/*
> + * Returns true if a send should try to push new data.
> + */
> +static int send_should_push(struct sock *sk, int flags)
> +{
> + return should_push(sk) && !corked(tcp_sk(sk), flags);
> +}

If it returns true, I guess it should be a bool.

> [...]

-- 
Stefano

Re: [PATCH v13 net-next 08/12] crypto : chtls - CPL handler definition

[Stefano Brivio]

On Tue, 27 Mar 2018 23:06:37 +0530
Atul Gupta <atul.gu...@chelsio.com> wrote:

> Exchange messages with hardware to program the TLS session
> CPL handlers for messages received from chip.
> 
> Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>
> Signed-off-by: Michael Werner <wer...@chelsio.com>
> Reviewed-by: Sabrina Dubroca <sdubr...@redhat.com>
> Reviewed-by: Stefano Brivio <sbri...@redhat.com>

No, I haven't.

-- 
Stefano

Re: [PATCH v13 net-next 09/12] crypto: chtls - Inline TLS record Tx

[Stefano Brivio]

On Tue, 27 Mar 2018 23:06:38 +0530
Atul Gupta <atul.gu...@chelsio.com> wrote:

> TLS handler for record transmit.
> Create Inline TLS work request and post to FW.
> Create Inline TLS record CPLs for hardware
> 
> Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>
> Signed-off-by: Michael Werner <wer...@chelsio.com>
> Reviewed-by: Stefano Brivio <sbri...@redhat.com>

Absolutely not.

-- 
Stefano

Re: [PATCH v13 net-next 07/12] crypto: chtls - Program the TLS session Key

[Stefano Brivio]

On Tue, 27 Mar 2018 23:06:36 +0530
Atul Gupta <atul.gu...@chelsio.com> wrote:

> Initialize the space reserved for storing the TLS keys,
> get and free the location where key is stored for the TLS
> connection.
> Program the Tx and Rx key as received from user in
> struct tls12_crypto_info_aes_gcm_128 and understood by hardware.
> added socket option TLS_RX
> 
> Signed-off-by: Atul Gupta <atul.gu...@chelsio.com>
> Reviewed-by: Sabrina Dubroca <sdubr...@redhat.com>
> Reviewed-by: Stefano Brivio <sbri...@redhat.com>

Nope, sorry.

-- 
Stefano

Re: [PATCH v12 net-next 06/12] crypto: chtls - structure and macro for Inline TLS

[Stefano Brivio]

On Mon, 19 Mar 2018 19:25:39 +0530
Atul Gupta  wrote:

> +#define SOCK_INLINE (31)
>
> [...]
>
> +static inline int csk_flag(const struct sock *sk, enum csk_flags flag)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +
> + if (!sock_flag(sk, SOCK_INLINE))
> + return 0;

Please take care of the comments.

I understand this series is big and you might be tempted to minimize
your effort in the hope that reviewers will fail to check that you
addressed their concerns.

You are also succeeding in making it hard by re-posting so quickly,
addressing a few "easy" comments at a time, without taking care of the
change log.

But still I believe your hopes are not so reasonably founded.

-- 
Stefano

Re: [PATCH v12 net-next 10/12] crypto: chtls - Inline TLS record Rx

[Stefano Brivio]

On Mon, 19 Mar 2018 19:25:43 +0530
Atul Gupta  wrote:

> +int chtls_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> +   int nonblock, int flags, int *addr_len)
> +{
>
> [...]
>
> + if (likely(!(flags & MSG_TRUNC))) {
> + if (skb_copy_datagram_msg(skb, offset,
> +   msg, avail)) {
> + if (!copied) {
> + copied = -EFAULT;
> + break;
> + }

Why is this change not mentioned in the series changelog?

-- 
Stefano

Re: [PATCH v12 net-next 01/12] tls: support for Inline tls record

[Stefano Brivio]

On Mon, 19 Mar 2018 19:25:34 +0530
Atul Gupta  wrote:

> @@ -268,6 +259,8 @@ static void tls_sk_proto_close(struct sock *sk, long 
> timeout)
>  skip_tx_cleanup:
>   release_sock(sk);
>   sk_proto_close(sk, timeout);
> + if (ctx && ctx->tx_conf == TLS_HW_RECORD)
> + kfree(ctx);

Why are you still dereferencing ctx after it has been freed, as
reported by Julia () on v11?

-- 
Stefano

Re: [PATCH v12 net-next 09/12] crypto: chtls - Inline TLS record Tx

[Stefano Brivio]

On Mon, 19 Mar 2018 19:25:42 +0530
Atul Gupta  wrote:

> +static bool is_tls_skb(struct chtls_sock *csk, const struct sk_buff *skb)
> +{
> + return skb_ulp_tls_skb_flags(skb);
> +}

Do you need this function?

> +/* Copy Key to WR */
> +static void tls_copy_tx_key(struct sock *sk, struct sk_buff *skb)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct ulptx_sc_memrd *sc_memrd;
> + struct ulptx_idata *sc;
> + struct chtls_hws *hws = >tlshws;
> + struct chtls_dev *cdev = csk->cdev;
> + u32 immdlen;
> + int kaddr;

Reverse christmas tree comments (that you already received for several
versions) ignored.

> [...]
>
> +/*
> + * Returns true if an sk_buff carries urgent data.
> + */
> +static bool skb_urgent(struct sk_buff *skb)
> +{
> + return (ULP_SKB_CB(skb)->flags & ULPCB_FLAG_URG) != 0;
> +}

You silently ignored most of the comment I had about this already on
v10.

> [...]
>
> +static void tls_tx_data_wr(struct sock *sk, struct sk_buff *skb,
> +int dlen, int tls_immd, u32 credits,
> +int expn, int pdus)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct chtls_hws *hws = >tlshws;
> + struct net_device *dev = csk->egress_dev;
> + struct adapter *adap = netdev2adap(dev);
> + struct fw_tlstx_data_wr *req_wr;
> + struct cpl_tx_tls_sfo *req_cpl;
> + int iv_imm = skb_ulp_tls_skb_iv(skb);
> + struct tls_scmd *scmd = >scmd;
> + struct tls_scmd *updated_scmd;
> + unsigned int wr_ulp_mode_force;
> + unsigned char data_type;
> + unsigned char *req;
> + int len = dlen + expn;
> + int immd_len;

Reverse christmas tree.

> +static int chtls_expansion_size(struct sock *sk, int data_len,
> + int fullpdu,
> + unsigned short *pducnt)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct chtls_hws *hws = >tlshws;
> + struct tls_scmd *scmd = >scmd;
> + int hdrlen = TLS_HEADER_LENGTH;
> + int expnsize, frcnt, fraglast, fragsize;
> + int expppdu;

Same here.

> +
> + do {
> + fragsize = hws->mfs;
> +
> + if (SCMD_CIPH_MODE_G(scmd->seqno_numivs) ==
> + SCMD_CIPH_MODE_AES_GCM) {
> + frcnt = (data_len / fragsize);
> + expppdu = GCM_TAG_SIZE + AEAD_EXPLICIT_DATA_SIZE +
> + hdrlen;
> + expnsize =  frcnt * expppdu;
> +
> + if (fullpdu) {
> + *pducnt = data_len / (expppdu + fragsize);
> +
> + if (*pducnt > 32)
> + *pducnt = 32;
> + else if (!*pducnt)
> + *pducnt = 1;
> + expnsize = (*pducnt) * expppdu;
> + break;

Was this supposed to be conditional?

> + }
> + fraglast = data_len % fragsize;
> + if (fraglast > 0) {
> + frcnt += 1;
> + expnsize += expppdu;
> + }
> + break;
> + }
> + pr_info("unsupported cipher\n");
> + expnsize = 0;
> + } while (0);

And why the "do { ... } while (0)" anyway? Copied from a macro?

This series is *still* full of this kind of stuff. Is it such a bizarre
request to ask you to go through your code line by line and check it
carefully? Why should others do it instead?

> +
> + return expnsize;
> +}
> +
> +/* WR with IV, KEY and CPL SFO added */
> +static void make_tlstx_data_wr(struct sock *sk, struct sk_buff *skb,
> +int tls_tx_imm, int tls_len, u32 credits)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct chtls_hws *hws = >tlshws;
> + int pdus = DIV_ROUND_UP(tls_len, hws->mfs);
> + unsigned short pdus_per_ulp = 0;
> + int expn_sz;

Reverse christmas tree comments ignored here.

> [...]
> +int chtls_push_frames(struct chtls_sock *csk, int comp)
> +{
> + struct sock *sk = csk->sk;
> + struct tcp_sock *tp = tcp_sk(sk);
> + struct chtls_hws *hws = >tlshws;
> + struct sk_buff *skb;
> + int total_size = 0;
> + int wr_size = sizeof(struct fw_ofld_tx_data_wr);

And here.

> [...]
> +int chtls_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct tcp_sock *tp = tcp_sk(sk);
> + struct chtls_dev *cdev = csk->cdev;
> + struct sk_buff *skb;
> + int mss, flags, err;
> + int copied = 0;
> + int hdrlen = 0;
> + int recordsz = 0;
> + long timeo;

And here.

> [...]
> +int chtls_sendpage(struct sock *sk, struct 

Re: [PATCH v12 net-next 00/12] Chelsio Inline TLS

[Stefano Brivio]

On Mon, 19 Mar 2018 19:25:33 +0530
Atul Gupta  wrote:

> Series for Chelsio Inline TLS driver (chtls)
> 
> [...]
>
> v12: patch against net-next
> - fixed few build error
> - replace set_queue with skb_set_queue_mapping [Sabrina]
> - copyright year correction

Please give at least an indication of the patches you're changing, it's
almost impossible to follow otherwise. Please take care of the comments.
Please report the changes you make here.

-- 
Stefano

Re: [PATCH v10 crypto 07/11] chtls: Program the TLS Key

[Stefano Brivio]

On Sat, 10 Mar 2018 00:40:12 +0530
Atul Gupta  wrote:

> Initialize the space reserved for storing the TLS keys
> get and free the location where key is stored for the TLS
> connection
> Program the tx and rx key as received from user in
> struct tls12_crypto_info_aes_gcm_128 and understood by hardware.
> 
> Signed-off-by: Atul Gupta 
> ---
>  drivers/crypto/chelsio/chtls/chtls_hw.c | 371 
> 
>  1 file changed, 371 insertions(+)
>  create mode 100644 drivers/crypto/chelsio/chtls/chtls_hw.c
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_hw.c 
> b/drivers/crypto/chelsio/chtls/chtls_hw.c
> new file mode 100644
> index 000..40cf84f
> --- /dev/null
> +++ b/drivers/crypto/chelsio/chtls/chtls_hw.c
> @@ -0,0 +1,371 @@
> +/*
> + * Copyright (c) 2017 Chelsio Communications, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Written by: Atul Gupta (atul.gu...@chelsio.com)
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "chtls.h"
> +#include "chtls_cm.h"
> +
> +static void __set_tcb_field_direct(struct chtls_sock *csk,
> +struct cpl_set_tcb_field *req, u16 word,
> +u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> + struct ulptx_idata *sc;
> +
> + INIT_TP_WR_CPL(req, CPL_SET_TCB_FIELD, csk->tid);
> + req->wr.wr_mid |= htonl(FW_WR_FLOWID_V(csk->tid));
> + req->reply_ctrl = htons(NO_REPLY_V(no_reply) |
> + QUEUENO_V(csk->rss_qid));
> + req->word_cookie = htons(TCB_WORD_V(word) | TCB_COOKIE_V(cookie));
> + req->mask = cpu_to_be64(mask);
> + req->val = cpu_to_be64(val);
> + sc = (struct ulptx_idata *)(req + 1);
> + sc->cmd_more = htonl(ULPTX_CMD_V(ULP_TX_SC_NOOP));
> + sc->len = htonl(0);
> +}
> +
> +static void __set_tcb_field(struct sock *sk, struct sk_buff *skb, u16 word,
> + u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> +
> + req = (struct cpl_set_tcb_field *)__skb_put(skb, wrlen);
> + __set_tcb_field_direct(csk, req, word, mask, val, cookie, no_reply);
> + set_wr_txq(skb, CPL_PRIORITY_CONTROL, csk->port_id);
> +}
> +
> +/*
> + * Send control message to HW, message go as immediate data and packet
> + * is freed immediately.
> + */
> +static void chtls_set_tcb_field(struct sock *sk, u16 word, u64 mask, u64 val)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + struct sk_buff *skb;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> + unsigned int credits_needed = DIV_ROUND_UP(wrlen, 16);
> +
> + skb = alloc_skb(wrlen, GFP_ATOMIC);
> + if (!skb)
> + return;
> +
> + __set_tcb_field(sk, skb, word, mask, val, 0, 1);
> + set_queue(skb, (csk->txq_idx << 1) | CPL_PRIORITY_DATA, sk);
> + csk->wr_credits -= credits_needed;
> + csk->wr_unacked += credits_needed;
> + enqueue_wr(csk, skb);
> + cxgb4_ofld_send(csk->egress_dev, skb);
> +}

What happens if e.g. alloc_skb() fails here? Is it fine to ignore the
error altogether?

> +
> +/*
> + * Set one of the t_flags bits in the TCB.
> + */
> +void chtls_set_tcb_tflag(struct sock *sk, unsigned int bit_pos, int val)
> +{
> + return chtls_set_tcb_field(sk, 1, 1ULL << bit_pos,
> + val << bit_pos);
> +}
> +
> +static void chtls_set_tcb_keyid(struct sock *sk, int keyid)
> +{
> + return chtls_set_tcb_field(sk, 31, 0xULL, keyid);
> +}
> +
> +static void chtls_set_tcb_seqno(struct sock *sk)
> +{
> + return chtls_set_tcb_field(sk, 28, ~0ULL, 0);
> +}
> +
> +static void chtls_set_tcb_quiesce(struct sock *sk, int val)
> +{
> + return chtls_set_tcb_field(sk, 1, (1ULL << TF_RX_QUIESCE_S),
> +TF_RX_QUIESCE_V(val));
> +}

Why would you return from a void function?

IMHO, it would be more productive if you could address the comments you
are receiving a bit more carefully instead of patching them up quickly
and race to re-post, because this doesn't seem to gain any time.

-- 
Stefano

Re: [PATCH v10 crypto 09/11] chtls: Inline TLS request Tx/Rx

[Stefano Brivio]

On Sat, 10 Mar 2018 00:40:14 +0530
Atul Gupta  wrote:

> TLS handler for record transmit and receive.
> Create Inline TLS work request and post to FW.
> Create Inline TLS record CPLs for hardware
> 
> Signed-off-by: Atul Gupta 
> ---
>  drivers/crypto/chelsio/chtls/chtls_io.c | 1863 
> +++
>  1 file changed, 1863 insertions(+)
>  create mode 100644 drivers/crypto/chelsio/chtls/chtls_io.c
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c 
> b/drivers/crypto/chelsio/chtls/chtls_io.c
> new file mode 100644
> index 000..f7f5826
> --- /dev/null
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -0,0 +1,1863 @@
> +/*
> + * Copyright (c) 2017 Chelsio Communications, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Written by: Atul Gupta (atul.gu...@chelsio.com)
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "chtls.h"
> +#include "chtls_cm.h"
> +
> +static bool is_tls_hw(struct chtls_sock *csk)
> +{
> + return csk->tlshws.ofld;
> +}

Do you actually need this function?

> +static bool is_tls_rx(struct chtls_sock *csk)
> +{
> + return (csk->tlshws.rxkey >= 0);
> +}

You can drop the ().

> +
> +static bool is_tls_tx(struct chtls_sock *csk)
> +{
> + return (csk->tlshws.txkey >= 0);
> +}

You can drop the ().

> +static bool is_tls_skb(struct chtls_sock *csk, const struct sk_buff *skb)
> +{
> + return (is_tls_hw(csk) && skb_ulp_tls_skb_flags(skb));
> +}

You can drop the ().

> +static int key_size(void *sk)
> +{
> + return 16; /* Key on DDR */
> +}

Do you actually need this function? Can't you turn that into a #define?

> +
> +#define ceil(x, y) \
> + ({ unsigned long __x = (x), __y = (y); (__x + __y - 1) / __y; })

This doesn't look like a ceiling function, could you perhaps name it
more descriptively?

> +static int data_sgl_len(const struct sk_buff *skb)
> +{
> + unsigned int cnt;
> +
> + cnt = skb_shinfo(skb)->nr_frags;
> + return (sgl_len(cnt) * 8);

You can drop the ().

> +}
> +
> +static int nos_ivs(struct sock *sk, unsigned int size)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +
> + return ceil(size, csk->tlshws.mfs);
> +}
> +
> +#define TLS_WR_CPL_LEN \
> + (sizeof(struct fw_tlstx_data_wr) + \
> + sizeof(struct cpl_tx_tls_sfo))
> +
> +static int is_ivs_imm(struct sock *sk, const struct sk_buff *skb)

Shouldn't this be a bool?

> +{
> + int ivs_size = nos_ivs(sk, skb->len) * CIPHER_BLOCK_SIZE;
> + int hlen = TLS_WR_CPL_LEN + data_sgl_len(skb);
> +
> + if ((hlen + key_size(sk) + ivs_size) <
> + MAX_IMM_OFLD_TX_DATA_WR_LEN) {
> + ULP_SKB_CB(skb)->ulp.tls.iv = 1;
> + return 1;
> + }
> + ULP_SKB_CB(skb)->ulp.tls.iv = 0;

Are these assignments intended? They don't seem to fit with the name of
the function.

> + return 0;
> +}
> +
> +static int max_ivs_size(struct sock *sk, int size)
> +{
> + return (nos_ivs(sk, size) * CIPHER_BLOCK_SIZE);
> +}

You can drop the ().

> +
> +static int ivs_size(struct sock *sk, const struct sk_buff *skb)
> +{
> + return (is_ivs_imm(sk, skb) ? (nos_ivs(sk, skb->len) *
> +  CIPHER_BLOCK_SIZE) : 0);
> +}

You can drop the ().

> [...]
>
> +static u64 tls_sequence_number(struct chtls_hws *hws)
> +{
> + return (hws->tx_seq_no++);
> +}

You can drop the (), and I find the name of this function a bit misleading
as you are actually increasing the sequence number, not just returning
it.

> +
> +static int is_sg_request(const struct sk_buff *skb)
> +{
> + return (skb->peeked ||
> + (skb->len > MAX_IMM_ULPTX_WR_LEN));
> +}

You can drop the (). This should be a bool.

> +
> +/*
> + * Returns true if an sk_buff carries urgent data.
> + */
> +static int skb_urgent(struct sk_buff *skb)
> +{
> + return (ULP_SKB_CB(skb)->flags & ULPCB_FLAG_URG) != 0;
> +}

This should be a bool, you can drop the (), and avoid that != 0
comparison.

-- 
Stefano

Re: [crypto 4/8] chtls: CPL handler definition

[Stefano Brivio]

Hi Atul,

On Thu, 7 Dec 2017 14:50:37 +
Atul Gupta <atul.gu...@chelsio.com> wrote:

> -Original Message-
> From: linux-crypto-ow...@vger.kernel.org 
> [mailto:linux-crypto-ow...@vger.kernel.org] On Behalf Of Stefano Brivio
> Sent: Tuesday, December 5, 2017 8:54 PM
> To: Atul Gupta <atul.gu...@chelsio.com>
> Cc: herb...@gondor.apana.org.au; linux-crypto@vger.kernel.org; 
> net...@vger.kernel.org; da...@davemloft.net; davejwat...@fb.com; Ganesh GR 
> <ganes...@chelsio.com>; Harsh Jain <ha...@chelsio.com>
> Subject: Re: [crypto 4/8] chtls: CPL handler definition

First off, it would help immensely if you used an e-mail client with
sane settings for line lengths limit and quoting as described by
RFC3676. Otherwise, this will get quite unreadable, quite soon.

> [...]
>
> > +void get_tcp_symbol(void)
> > +{
> > +   tcp_time_wait_p = (void *)kallsyms_lookup_name("tcp_time_wait");
> > +   if (!tcp_time_wait_p)
> > +   pr_info("could not locate tcp_time_wait");  
> 
> Probably not something that should be used here. Why do you need this?
> [Atul] using it to call tcp_time_wait, as used in tcp_rcv_state_process

Indeed, but why do you need to call tcp_time_wait() directly by looking
it up by symbol name, especially from a network driver? This is really
against any kind of accepted API practice or architecture consideration
whatsoever.

> [...]
>
> > +int chtls_send_reset(struct sock *sk, int mode, struct sk_buff *skb)
> > +{
> > +   struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> > +
> > +   if (unlikely(csk_flag_nochk(csk, CSK_ABORT_SHUTDOWN) ||
> > +!csk->cdev)) {
> > +   if (sk->sk_state == TCP_SYN_RECV)
> > +   csk_set_flag(csk, CSK_RST_ABORTED);
> > +   goto out;
> > +   }
> > +
> > +   if (!csk_flag_nochk(csk, CSK_TX_DATA_SENT)) {
> > +   struct tcp_sock *tp = tcp_sk(sk);
> > +
> > +   if (send_tx_flowc_wr(sk, 0, tp->snd_nxt, tp->rcv_nxt) < 0)
> > +   WARN_ONCE(1, "send tx flowc error");
> > +   csk_set_flag(csk, CSK_TX_DATA_SENT);
> > +   }
> > +
> > +   csk_set_flag(csk, CSK_ABORT_RPL_PENDING);
> > +   chtls_purge_write_queue(sk);
> > +
> > +   csk_set_flag(csk, CSK_ABORT_SHUTDOWN);
> > +   if (sk->sk_state != TCP_SYN_RECV)
> > +   chtls_send_abort(sk, mode, skb);  
> 
> If sk->sk_state == TCP_SYN_RECV, aren't we leaking skb, coming e.g.
> from reset_listen_child()?
> [Atul] If (sk->sk_state == TCP_SYN_RECV) we free the skb, else we call the 
> send abort where skb is freed on completion.

That will only happen if, additionally:

csk_flag_nochk(csk, CSK_ABORT_SHUTDOWN) || !csk->cdev

but otherwise, you can probably end up here with (sk->sk_state ==
TCP_SYN_RECV) and leak the skb.

> [...]
> > +int chtls_listen_start(struct chtls_dev *cdev, struct sock *sk)
>
> [...]
>
> > +   if (cdev->lldi->enable_fw_ofld_conn) {
> > +   ret = cxgb4_create_server_filter(ndev, stid,
> > +inet_sk(sk)->inet_rcv_saddr,
> > +inet_sk(sk)->inet_sport, 0,
> > +cdev->lldi->rxq_ids[0], 0, 0);
> > +   } else {
> > +   ret = cxgb4_create_server(ndev, stid,
> > + inet_sk(sk)->inet_rcv_saddr,
> > + inet_sk(sk)->inet_sport, 0,
> > + cdev->lldi->rxq_ids[0]);
> > +   }
> > +   if (ret > 0)
> > +   ret = net_xmit_errno(ret);
> > +   if (ret)
> > +   goto del_hash;
> > +
> > +   if (!ret)  
> 
> Not needed I guess?
> [Atul] its required, cxgb4_create_server calls net_xmit_eval where ret can be 
> NET_XMIT_SUCCESS/DROP/CN. 
> net_xmit_eval can return 0 or 1.
> If 1, net_xmit_errno is called which returns ENOBUF or 0. If ENOBUF goto 
> del_hash else return 0

You are doing something like:

if (x)
goto y;
if (!x)
return 0;
y:

hence the if (!x) clause appears indeed to be quite useless, because
you will never reach that clause if 'x' is true, which voids the whole
purpose of checking whether it's false.

> [...]
> > +static struct sock *chtls_recv_sock(struct sock *lsk,
> > +   struct request_sock *oreq,
> > +   void *network_hdr,
> > +   const struct cpl_pass_accept_req *req,

Re: [crypto 4/8] chtls: CPL handler definition

[Stefano Brivio]

On Tue,  5 Dec 2017 17:10:00 +0530
Atul Gupta  wrote:

> CPL handlers for TLS session, record transmit and receive
> 
> Signed-off-by: Atul Gupta 
> ---
>  drivers/crypto/chelsio/chtls/chtls_cm.c | 2048 
> +++
>  1 file changed, 2048 insertions(+)
>  create mode 100644 drivers/crypto/chelsio/chtls/chtls_cm.c
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_cm.c 
> b/drivers/crypto/chelsio/chtls/chtls_cm.c
> new file mode 100644
> index 000..ea1c301
> --- /dev/null
> +++ b/drivers/crypto/chelsio/chtls/chtls_cm.c
> @@ -0,0 +1,2048 @@
> +/*
> + * Copyright (c) 2017 Chelsio Communications, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Written by: Atul Gupta (atul.gu...@chelsio.com)
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "chtls.h"
> +#include "chtls_cm.h"
> +
> +extern struct request_sock_ops chtls_rsk_ops;
> +static void (*tcp_time_wait_p)(struct sock *sk, int state, int timeo);
> +
> +/*
> + * State transitions and actions for close.  Note that if we are in SYN_SENT
> + * we remain in that state as we cannot control a connection while it's in
> + * SYN_SENT; such connections are allowed to establish and are then aborted.
> + */
> +static unsigned char new_state[16] = {
> + /* current state: new state:  action: */
> + /* (Invalid)   */ TCP_CLOSE,
> + /* TCP_ESTABLISHED */ TCP_FIN_WAIT1 | TCP_ACTION_FIN,
> + /* TCP_SYN_SENT*/ TCP_SYN_SENT,
> + /* TCP_SYN_RECV*/ TCP_FIN_WAIT1 | TCP_ACTION_FIN,
> + /* TCP_FIN_WAIT1   */ TCP_FIN_WAIT1,
> + /* TCP_FIN_WAIT2   */ TCP_FIN_WAIT2,
> + /* TCP_TIME_WAIT   */ TCP_CLOSE,
> + /* TCP_CLOSE   */ TCP_CLOSE,
> + /* TCP_CLOSE_WAIT  */ TCP_LAST_ACK | TCP_ACTION_FIN,
> + /* TCP_LAST_ACK*/ TCP_LAST_ACK,
> + /* TCP_LISTEN  */ TCP_CLOSE,
> + /* TCP_CLOSING */ TCP_CLOSING,
> +};
> +
> +static struct chtls_sock *chtls_sock_create(struct chtls_dev *cdev)
> +{
> + struct chtls_sock *csk = kzalloc(sizeof(*csk), GFP_NOIO);
> +
> + if (!csk)
> + return NULL;
> +
> + csk->txdata_skb_cache =  alloc_skb(TXDATA_SKB_LEN, GFP_ATOMIC);

Excess whitespace.

> + if (!csk->txdata_skb_cache) {
> + kfree(csk);
> + return NULL;
> + }
> +
> + kref_init(>kref);
> + csk->cdev = cdev;
> + skb_queue_head_init(>txq);
> + csk->wr_skb_head = NULL;
> + csk->wr_skb_tail = NULL;
> + csk->mss = MAX_MSS;
> + csk->tlshws.ofld = 1;
> + csk->tlshws.txkey = -1;
> + csk->tlshws.rxkey = -1;
> + csk->tlshws.mfs = TLS_MFS;
> + skb_queue_head_init(>tlshws.sk_recv_queue);
> + return csk;
> +}
> +
> +void chtls_sock_release(struct kref *ref)
> +{
> + struct chtls_sock *csk =
> + container_of(ref, struct chtls_sock, kref);
> +
> + kfree(csk);
> +}
> +
> +void get_tcp_symbol(void)
> +{
> + tcp_time_wait_p = (void *)kallsyms_lookup_name("tcp_time_wait");
> + if (!tcp_time_wait_p)
> + pr_info("could not locate tcp_time_wait");

Probably not something that should be used here. Why do you need this?

> +}
> +
> +static struct net_device *chtls_ipv4_netdev(struct chtls_dev *cdev,
> + struct sock *sk)
> +{
> + struct net_device *ndev = cdev->ports[0];
> +
> + if (likely(!inet_sk(sk)->inet_rcv_saddr))
> + return ndev;
> +
> + ndev = ip_dev_find(_net, inet_sk(sk)->inet_rcv_saddr);
> + if (!ndev)
> + return NULL;
> +
> + if (is_vlan_dev(ndev))
> + return vlan_dev_real_dev(ndev);
> + return ndev;
> +}
> +
> +static void assign_rxopt(struct sock *sk, unsigned int opt)
> +{
> + const struct chtls_dev *cdev;
> + struct tcp_sock *tp = tcp_sk(sk);
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);

Reverse christmas tree format?

> +
> + cdev = csk->cdev;
> + tp->tcp_header_len   = sizeof(struct tcphdr);
> + tp->rx_opt.mss_clamp = cdev->mtus[TCPOPT_MSS_G(opt)] - 40;
> + tp->mss_cache= tp->rx_opt.mss_clamp;
> + tp->rx_opt.tstamp_ok = TCPOPT_TSTAMP_G(opt);
> + tp->rx_opt.snd_wscale= TCPOPT_SACK_G(opt);
> + tp->rx_opt.wscale_ok = TCPOPT_WSCALE_OK_G(opt);
> + SND_WSCALE(tp)   = TCPOPT_SND_WSCALE_G(opt);
> + if (!tp->rx_opt.wscale_ok)
> + tp->rx_opt.rcv_wscale = 0;
> + if (tp->rx_opt.tstamp_ok) {
> + tp->tcp_header_len += TCPOLEN_TSTAMP_ALIGNED;
> + tp->rx_opt.mss_clamp -= TCPOLEN_TSTAMP_ALIGNED;
> + } else if (csk->opt2