netdev - Re: [PATCH V6 net-next 2/2] smc: introduce socket family AF

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]

Date:	Mon, 21 Sep 2015 11:44:25 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	ubraun@...ux.vnet.ibm.com
Cc:	utz.bacher@...ibm.com, netdev@...r.kernel.org,
	linux-s390@...r.kernel.org, schwidefsky@...ibm.com,
	heiko.carstens@...ibm.com, ursula.braun@...ibm.com
Subject: Re: [PATCH V6 net-next 2/2] smc: introduce socket family AF_SMC

From: Ursula Braun <ubraun@...ux.vnet.ibm.com>
Date: Mon, 14 Sep 2015 12:42:46 +0200

> +#ifdef CONFIG_S390
> +#include <asm/ebcdic.h>
> +#endif

Please keep s390'isms out of this code.

> +void smc_link_up(struct smc_link_group *lgr)
> +{
> +	struct smc_roce_defs rocdefs;
> +	struct smc_link *link = NULL;
> +	int i;
> +	u8 p1, p2;

Please order local variable declarations from longest to shortest
line (aka: "reverse christimas tree").

Please fix this up for your entire submission, as I'm not going to
point out all of the instances explicitly.

> +
> +	/* determine working link */
> +	for (i = 0; i <= SMC_MAX_SYM_LINKS; i++) {
> +		if (atomic_read(&lgr->lnk[i].state) == SMC_LINK_UP) {
> +			link = &lgr->lnk[i];
> +			break;
> +		}
> +	}

Using an atomic_t type and then only performing 'set' and 'read'
operations on that variable is pointless and doesn't provide you
with any real "atomicity".

Please use a plain int, or provide a complete and suitable
justification for using atomic_t in this situation.

> +	for (i = 0; i < 2; i++) {

Please change this '2' constant in all of these loops to some
suitable mnenomic.  Likewise in the declaration of the 'port_err'
struct member et al.

> +		get_random_bytes_arch(&local_peerid[0], 2);

I really don't see any justification for using get_random_bytes_arch()
vs. plain get_random_bytes().

> +void smc_sock_wake_tx(struct sock *sk)
> +{
> +	struct smc_sock *smc = smc_sk(sk);
> +	struct socket_wq *wq;
> +	struct socket *sock = sk->sk_socket;
> +
> +/* similar to sk_stream_write_space */

This comment is imporperly indented.

> +		/* cancel close waiting */
> +		atomic64_set(&conn->tx_curs_prep.s.acurs,
> +			     conn->tx_curs_sent.s.lcurs);
> +		atomic64_set(&conn->tx_curs_fin.s.acurs,
> +			     conn->tx_curs_sent.s.lcurs);

Another case of atomic types being used inappropriately.  You only use
'set' and 'read' operations on these values, and that makes using
atomic types entirely pointless.

This is a huge and complex submission and I'm already burnt out
reading the changes thus far.

You have a lot to fix up and you can expect many revisions to be
necessary before these changes are ready for integration upstream.
And that's if you are lucky and someone actually continues to review
this work.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html