netdev - Re: [PATCH v13 net-next 09/12] crypto: chtls

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: <20180327195116.0ac14f7e@epycfail>
Date:   Tue, 27 Mar 2018 19:51:16 +0200
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Atul Gupta <atul.gupta@...lsio.com>
Cc:     davem@...emloft.net, herbert@...dor.apana.org.au,
        davejwatson@...com, sd@...asysnail.net,
        linux-crypto@...r.kernel.org, netdev@...r.kernel.org,
        werner@...lsio.com, leedom@...lsio.com,
        swise@...ngridcomputing.com, indranil@...lsio.com,
        ganeshgr@...lsio.com
Subject: Re: [PATCH v13 net-next 09/12] crypto: chtls - Inline TLS record Tx

On Tue, 27 Mar 2018 23:06:38 +0530
Atul Gupta <atul.gupta@...lsio.com> 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