netdev - RE: [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis

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:	Wed, 11 Dec 2013 10:20:32 -0000
From:	"David Laight" <David.Laight@...LAB.COM>
To:	"David Miller" <davem@...emloft.net>, <yangyingliang@...wei.com>
Cc:	<netdev@...r.kernel.org>
Subject: RE: [PATCH net-next 6/6] net_sched: make macro be enclosed in parenthesis

> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org] On Behalf Of David Miller
> From: Yang Yingliang <yangyingliang@...wei.com>
> Date: Tue, 10 Dec 2013 20:55:34 +0800
> 
> > Macros with complex values should be enclosed in parenthesis
> >
> > Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> > ---
> >  net/sched/em_meta.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
> > index e5cef956..852cd62 100644
> > --- a/net/sched/em_meta.c
> > +++ b/net/sched/em_meta.c
> > @@ -272,10 +272,12 @@ META_COLLECTOR(int_rtiif)
> >   **************************************************************************/
> >
> >  #define SKIP_NONLOCAL(skb)			\
> > +({						\
> >  	if (unlikely(skb->sk == NULL)) {	\
> >  		*err = -1;			\
> >  		return;				\
> > -	}
> > +	}					\
> > +})
> 
> I can't apply this.
> 
> First of all, "({ })" is for statements that evaluate to an lvalue,
> this macro does not.
> 
> Second of all, and more importantly, this macro needs to be eliminated
> entirely.  It completely hides control flow, and in the past we've
> killed macros which do this such as the old netlink attribute
> builders.
> 
> The control flow should be inlined and expanded explicitly in the
> code so that someone who reads it can tell the control flow can
> be changed by the statement.  Compare:
> 
> 	SKIP_NONLOCAL(skb)
> 
> to:
> 
> 	if (skip_nonlocal(skb)) {
> 		*err = -1;
> 		return;
> 	}

Or:
	if (!skb->sk)
		goto no_sk;
	...
	return;

no_sk:
	*err = -1;
	return;

Actually WTF is a void function doing having an 'int *err' argument?
These should just be:
	if (!skb->sk)
		return -1;

	David



--
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