netdev - Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers

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: <20190930153343.GE14745@lunn.ch>
Date:   Mon, 30 Sep 2019 17:33:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, idosch@...lanox.com,
        pabeni@...hat.com, edumazet@...gle.com, petrm@...lanox.com,
        sd@...asysnail.net, f.fainelli@...il.com,
        stephen@...workplumber.org, mlxsw@...lanox.com
Subject: Re: [patch net-next 2/3] net: introduce per-netns netdevice notifiers

On Mon, Sep 30, 2019 at 04:23:49PM +0200, Jiri Pirko wrote:
> Mon, Sep 30, 2019 at 03:38:24PM CEST, andrew@...n.ch wrote:
> >>  static int call_netdevice_notifiers_info(unsigned long val,
> >>  					 struct netdev_notifier_info *info)
> >>  {
> >> +	struct net *net = dev_net(info->dev);
> >> +	int ret;
> >> +
> >>  	ASSERT_RTNL();
> >> +
> >> +	/* Run per-netns notifier block chain first, then run the global one.
> >> +	 * Hopefully, one day, the global one is going to be removed after
> >> +	 * all notifier block registrators get converted to be per-netns.
> >> +	 */
> >
> >Hi Jiri
> >
> >Is that really going to happen? register_netdevice_notifier() is used
> >in 130 files. Do you plan to spend the time to make it happen?
> 
> That's why I prepended the sentency with "Hopefully, one day"...
> 
> 
> >
> >> +	ret = raw_notifier_call_chain(&net->netdev_chain, val, info);
> >> +	if (ret & NOTIFY_STOP_MASK)
> >> +		return ret;
> >>  	return raw_notifier_call_chain(&netdev_chain, val, info);
> >>  }
> >
> >Humm. I wonder about NOTIFY_STOP_MASK here. These are two separate
> >chains. Should one chain be able to stop the other chain? Are there
> 
> Well if the failing item would be in the second chain, at the beginning
> of it, it would be stopped too. Does not matter where the stop happens,
> the point is that the whole processing stops. That is why I added the
> check here.
> 
> 
> >other examples where NOTIFY_STOP_MASK crosses a chain boundary?
> 
> Not aware of it, no. Could you please describe what is wrong?

You are expanding the meaning of NOTIFY_STOP_MASK. It now can stop
some other chain. If this was one chain with a filter, i would not be
asking. But this is two different chains, and one chain can stop
another? At minimum, i think this needs to be reviewed by the core
kernel people.

But i'm also wondering if you are solving the problem at the wrong
level. Are there other notifier chains which would benefit from
respecting name space boundaries? Would a better solution be to extend
struct notifier_block with some sort of filter?

Do you have some performance numbers? Where are you getting your
performance gains from? By the fact you are doing NOTIFY_STOP_MASK
earlier, so preventing a long chain being walked? I notice
notifer_block has a priority field. Did you try using that to put your
notified earlier on the chain?

	 Andrew