netdev - Re: [PATCH V2] Add flow control to the portmapper

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: <20160722152601.GA77728@ssaleem-MOBL4.amr.corp.intel.com>
Date:	Fri, 22 Jul 2016 10:26:01 -0500
From:	Shiraz Saleem <shiraz.saleem@...el.com>
To:	Leon Romanovsky <leon@...nel.org>
Cc:	dledford@...hat.com, linux-rdma@...r.kernel.org,
	swise@...ngridcomputing.com, e1000-rdma@...ts.sourceforge.net,
	netdev@...r.kernel.org, Mustafa Ismail <mustafa.ismail@...el.com>
Subject: Re: [PATCH V2] Add flow control to the portmapper

On Thu, Jul 21, 2016 at 08:29:42PM +0300, Leon Romanovsky wrote:
> On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > 
> > > > > You are the one user of this new inline function.
> > > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > > without messing with widely visible header file?
> > > > 
> > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> > > > to make a blocking version available to others as well as maintain 
> > > > consistency of existing code.
> > > > 
> > > 
> > > In such way, please provide patch series which will convert all other
> > > users to this new call.
> > > 
> > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);
> > 
> > These usages of netlink_unicast() with blocking are not the same as the new
> > nlmsg_unicast_block() function. 
> 
> Really?
> Did you look in the code?
> Let's take first function from that grep output
> 
> 414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> 415         if (err < 0) {
> 			... do something ...
> 437         } else
> 			... do something else ...
> 
> which fits nicely with your proposal.
> 
> +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32 portid)
> +{
> +       int err;
> +
> +       err = netlink_unicast(sk, skb, portid, 0);
> +       if (err > 0)
> +               err = 0;
> +
> +       return err;
> +}
> 
> 
> > You can't drop in nlmsg_unicast_block() in 
> > place of netlink_unicast() in these places. I'm not going to introduce code 
> > which modifies old behavior.
> 
> Again, you aren't changing any behaviour.
> Anyway we are not adding general function to common include file just
> because one caller wants it.
> 

We assumed the nlmsg_ API in linux/include/net/netlink.h is there for a purpose. 
That purpose is to normalize the return code. That API is used in places where 
the return code needs to be normalized, and when normalization is not needed, 
then the direct calls are used. 

Now since the nlm_ API in netlink.h is missing a blocking version of the 
nlmsg_unicast function, it would seem reasonable to add it there.

Changing all the direct calls as you suggest would at the very least be 
less efficient since it would normalize return codes when not needed. 

However, if there is a strict rule against adding an API unless you immediately 
have at least 2 callers, then I guess, we will make the direct call. The amount 
of code added will be the same, except that the next person who wants a normalized 
return code will have to duplicate the same code.

Changing other code to be less efficient so that we can meet the 2 caller criteria 
doesn't seem reasonable.