linux-kernel - Re: [PATCH net 1/1 v4] rtnetlink: require unique netns identifier

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: <20180207145326.7e4adf24@redhat.com>
Date:   Wed, 7 Feb 2018 14:53:26 +0100
From:   Jiri Benc <jbenc@...hat.com>
To:     Christian Brauner <christian.brauner@...onical.com>
Cc:     Kirill Tkhai <ktkhai@...tuozzo.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        netdev@...r.kernel.org, stephen@...workplumber.org,
        w.bumiller@...xmox.com, ebiederm@...ssion.com,
        nicolas.dichtel@...nd.com, linux-kernel@...r.kernel.org,
        dsahern@...il.com, davem@...emloft.net
Subject: Re: [PATCH net 1/1 v4] rtnetlink: require unique netns identifier

On Wed, 7 Feb 2018 14:36:21 +0100, Christian Brauner wrote:
> On Wed, Feb 07, 2018 at 04:20:01PM +0300, Kirill Tkhai wrote:
> > Can't we write these 3 above branches more compact? Something like this:
> > 
> > 	if (!!tb[IFLA_NET_NS_FD] + !!tb[IFLA_IF_NETNSID] + !!tb[IFLA_NET_NS_PID] <= 1)
> > 		return 0;  
> 
> I always prefer for conditions to be separate and readable even if it
> means additional code. But if others feel that there's value in avoiding
> two additional conditions I'm happy to adapt the patch.

FWIW, I don't like the n x n conditions much. But Kirill's proposal
seems not to be much better. I was thinking about:

	int cnt = 0;
	if (tb[IFLA_NET_NS_FD])
		cnt++;
	if (tb[IFLA_NET_NS_PID])
		cnt++;
	if (tb[IFLA_NET_NETNSID])
		cnt++;
	if (cnt > 1) {
		...errorr...
	}

but that's not better, either. As we're unlikely to add a fourth value,
I guess I'm okay with the current approach in the patch.

> Before I added support for netns ids for additional requests Jiri made
> it so that all request that specified properties that they did not
> support returned ENOTSUPP instead of EINVAL. This just keeps things
> consistent. Users would now suddenly receive EINVAL. That's potentially
> confusing.

Yes, please, keep -EOPNOTSUPP.

> As for the case of passing multiple netns identifying properties into
> the same request EINVAL seems the perfect candidate and the error
> message seems instructive to userspace programs.

Agreed.

Acked-by: Jiri Benc <jbenc@...hat.com>

Thanks,

 Jiri