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]

Date:	Mon, 25 Jul 2016 08:22:14 +0300
From:	Leon Romanovsky <leon@...nel.org>
To:	Steve Wise <swise@...ngridcomputing.com>
Cc:	'Shiraz Saleem' <shiraz.saleem@...el.com>, dledford@...hat.com,
	linux-rdma@...r.kernel.org, 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 12:42:42PM -0500, Steve Wise 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.
> >
> 
> The key is to ensure that places calling a blocking service are never called in a non-blocking context.   Leon, do you know if the new sites are always safe to block?  
> 
> In general, I think blocking due to sockbuf flow control vs dropping or retrying is a good thing for all the users in the rdam core, assuming they are safe to block.

Steve,
Sorry for my slow response,

I afraid that you was misled by the author of the proposed patch who did
two logical changes in one patch. One is move from non-blocking mode to
blocking mode which is fine enough after justification was added. And
the second change is introduction of general inline function in common
header file (include/net/netlink.h) with one caller only.

This second change is in question and I'm not feeling comfortable by
half done work.

> 
>  
> > +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.
> 
> Potential block/sleep is a change.  But if we can conclude that these additional sites are safe to block, then probably its ok to just go ahead and use the blocking service everywhere.

These potential sites has the same blocking call now netlink_unicast(... , ... , ... , 0),
the difference and question if they can handle normalized return value from new nlmsg_unicast_block
function. I'm convinced that the answer is yes.

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)