netdev - Re: [PATCH 2/2] mac80211_hwsim: Allow managing radios from non-initial namespaces

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: <1462350799.2546.31.camel@strongswan.org>
Date:	Wed, 04 May 2016 10:33:19 +0200
From:	Martin Willi <martin@...ongswan.org>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] mac80211_hwsim: Allow managing radios from
 non-initial namespaces

> > +static __net_init int hwsim_init_net(struct net *net)
> > +{
> > +	struct mac80211_hwsim_data *data;
> > +	bool exists = true;
> > +	int netgroup = 0;
> > +
> > +	spin_lock_bh(&hwsim_radio_lock);
> > +	while (exists) {
> > +		exists = false;
> > +		list_for_each_entry(data, &hwsim_radios, list) {
> > +			if (netgroup == data->netgroup) {
> > +				exists = true;
> > +				netgroup++;
> > +				break;
> > +			}
> > +		}
> > +	}
> > +	spin_unlock_bh(&hwsim_radio_lock);
> > +
> > +	*(int *)net_generic(net, hwsim_net_id) = netgroup;
> 
> This seems somewhat awkward. Why not just take the maximum of all the
> netgroup IDs + 1? We'd run out of memory and radio IDs long before
> netgroup IDs even that way

My intention was to reuse netgroups for the case many namespaces come
and go, but I agree that it is not optimal.

> consider a new netns that doesn't have any hwsim radios yet.
> Now you create *another* one, but it would get the same netgroup.

Correct, that is indeed broken if there are no radios.

> IOW, you should simply use a global counter. Surprising (net)
> namespaces don't have an index like that already, but I don't see
> one.

Ok, will do that in a v2.

> > +static void __net_exit hwsim_exit_net(struct net *net)
> > +{
> > +	struct mac80211_hwsim_data *entry, *tmp;
> > +
> > +	spin_lock_bh(&hwsim_radio_lock);
> > +	list_for_each_entry_safe(entry, tmp, &hwsim_radios, list) {
> > +		if (net_eq(wiphy_net(entry->hw->wiphy), net)) {
> > +			list_del(&entry->list);
> > +			INIT_WORK(&entry->destroy_work, destroy_radio);
> > +			schedule_work(&entry->destroy_work);
> > +		}
> > +	}
> > +	spin_unlock_bh(&hwsim_radio_lock);
> > +}

> This changes today's default behaviour of moving the wiphys to the
> default namespace. Did you intend to destroy them based on the
> netgroup, i.e. based on the namespace that created them? Actually,
> maybe they should move back to the namespace that created them, if
> the namespace they are in is destroyed? But that's difficult, I don't
> mind this behaviour, but I'm not sure it's what we want by default
> for radios created in the init_net.

With the proposed approach I destroy all radios if the owning namespace
gets deleted, because we probably don't want them landing in init_net
if they are created from a (unprivileged) userns process. I think this
is what other "virtual" interfaces do (gre tunnels, veth etc.). If we
think of hwsim radios as such a "virtual" device, that makes IMO sense
to delete them.

If we want to keep the existing behavior, we could move radios
belonging to the init_net-associated netgroup back to init_net, that
shouldn't be too difficult.

Moving the radio back to the creators namespace would be the most
consistent behavior, so I'll check how difficult such a reverse lookup
is. We then would delete the radio only if it is in the creators
namespace, or if the creators namespace is gone. Does that make sense?

Martin