[RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Denis Kenzior]

This patch allows GET_INTERFACE dumps to be filtered based on
NL80211_ATTR_WIPHY or NL80211_ATTR_WDEV.  The documentation for
GET_INTERFACE mentions that this is possible:
"Request an interface's configuration; either a dump request on
a %NL80211_ATTR_WIPHY or ..."

However, this behavior has not been implemented until now.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 net/wireless/nl80211.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 46417f9..ac19eb8 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2519,15 +2519,47 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag
 	return -EMSGSIZE;
 }
 
+static int nl80211_dump_interface_parse(struct sk_buff *skb,
+				    struct netlink_callback *cb,
+				    int *filter_wiphy)
+{
+	struct nlattr **tb = nl80211_fam.attrbuf;
+	int ret = nlmsg_parse(cb->nlh, GENL_HDRLEN + nl80211_fam.hdrsize,
+			      tb, nl80211_fam.maxattr, nl80211_policy);
+	/* ignore parse errors for backward compatibility */
+	if (ret)
+		return 0;
+
+	if (tb[NL80211_ATTR_WIPHY])
+		*filter_wiphy = nla_get_u32(tb[NL80211_ATTR_WIPHY]);
+	if (tb[NL80211_ATTR_WDEV])
+		*filter_wiphy = nla_get_u64(tb[NL80211_ATTR_WDEV]) >> 32;
+
+	return 0;
+}
+
 static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	int wp_idx = 0;
 	int if_idx = 0;
 	int wp_start = cb->args[0];
 	int if_start = cb->args[1];
+	int filter_wiphy = cb->args[2];
 	struct cfg80211_registered_device *rdev;
 	struct wireless_dev *wdev;
 
+	if (!wp_start && !if_start && !filter_wiphy) {
+		int ret;
+
+		filter_wiphy = -1;
+
+		ret = nl80211_dump_interface_parse(skb, cb, &filter_wiphy);
+		if (ret)
+			return ret;
+
+		cb->args[2] = filter_wiphy;
+	}
+
 	rtnl_lock();
 	list_for_each_entry(rdev, &cfg80211_rdev_list, list) {
 		if (!net_eq(wiphy_net(&rdev->wiphy), sock_net(skb->sk)))
@@ -2536,6 +2568,10 @@ static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback *
 			wp_idx++;
 			continue;
 		}
+
+		if (filter_wiphy != -1 && filter_wiphy != rdev->wiphy_idx)
+			continue;
+
 		if_idx = 0;
 
 		list_for_each_entry(wdev, &rdev->wiphy.wdev_list, list) {
-- 
2.7.3

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Johannes Berg]

On Wed, 2016-08-03 at 17:02 -0500, Denis Kenzior wrote:
> 
> +static int nl80211_dump_interface_parse(struct sk_buff *skb,
> +				    struct netlink_callback *cb,
> +				    int *filter_wiphy)

Wrong indentation :)

>  static int nl80211_dump_interface(struct sk_buff *skb, struct
> netlink_callback *cb)
>  {
>  	int wp_idx = 0;
>  	int if_idx = 0;
>  	int wp_start = cb->args[0];
>  	int if_start = cb->args[1];
> +	int filter_wiphy = cb->args[2];
>  	struct cfg80211_registered_device *rdev;
>  	struct wireless_dev *wdev;
>  
> +	if (!wp_start && !if_start && !filter_wiphy) {

This seems incorrect - you're setting

> +		int ret;
> +
> +		filter_wiphy = -1;
> +
> +		ret = nl80211_dump_interface_parse(skb, cb,
> &filter_wiphy);

it here, but it can take the value 0, so !filter_wiphy seems wrong?

johannes

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Denis Kenzior]

Hi Johannes,

On 08/11/2016 07:47 AM, Johannes Berg wrote:
> On Wed, 2016-08-03 at 17:02 -0500, Denis Kenzior wrote:
>>
>> +static int nl80211_dump_interface_parse(struct sk_buff *skb,
>> +				    struct netlink_callback *cb,
>> +				    int *filter_wiphy)
>
> Wrong indentation :)

Sorry :)

Speaking of indentation, can you point me to a doc of the rules I should 
follow?

>
>>   static int nl80211_dump_interface(struct sk_buff *skb, struct
>> netlink_callback *cb)
>>   {
>>   	int wp_idx = 0;
>>   	int if_idx = 0;
>>   	int wp_start = cb->args[0];
>>   	int if_start = cb->args[1];
>> +	int filter_wiphy = cb->args[2];
>>   	struct cfg80211_registered_device *rdev;
>>   	struct wireless_dev *wdev;
>>
>> +	if (!wp_start && !if_start && !filter_wiphy) {
>
> This seems incorrect - you're setting
>
>> +		int ret;
>> +
>> +		filter_wiphy = -1;
>> +
>> +		ret = nl80211_dump_interface_parse(skb, cb,
>> &filter_wiphy);
>
> it here, but it can take the value 0, so !filter_wiphy seems wrong?
>

I can confirm that I sanity checked this patch. Both ATTR_WIPHY, 
ATTR_WDEV and wildcard dumps seemed to produce expected results.

I noticed you applied this patch.  Is there a particular scenario where 
it goes wrong or did you convince yourself it is correct?

Regards,
-Denis

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Johannes Berg]

On Thu, 2016-08-11 at 11:38 -0500, Denis Kenzior wrote:
> Hi Johannes,
> 
> On 08/11/2016 07:47 AM, Johannes Berg wrote:
> > 
> > On Wed, 2016-08-03 at 17:02 -0500, Denis Kenzior wrote:
> > > 
> > > 
> > > +static int nl80211_dump_interface_parse(struct sk_buff *skb,
> > > +				    struct netlink_callback *cb,
> > > +				    int *filter_wiphy)
> > 
> > Wrong indentation :)
> 
> Sorry :)
> 
> Speaking of indentation, can you point me to a doc of the rules I
> should follow?

You've seen Documentation/CodingStyle?

> I can confirm that I sanity checked this patch. Both ATTR_WIPHY, 
> ATTR_WDEV and wildcard dumps seemed to produce expected results.

I think it probably works due to the other conditions?

> I noticed you applied this patch.  Is there a particular scenario
> where it goes wrong or did you convince yourself it is correct?
> 

No, that was a mistake :( I've removed it now.

johannes

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Denis Kenzior]

Hi Johannes,

 >> Speaking of indentation, can you point me to a doc of the rules I
>> should follow?
>
> You've seen Documentation/CodingStyle?

Of course.  But that one doesn't discuss that you want your function 
parameters to be aligned to the opening '('.  Is there a dialect 
document specific to linux-wireless?

>
>> I can confirm that I sanity checked this patch. Both ATTR_WIPHY,
>> ATTR_WDEV and wildcard dumps seemed to produce expected results.
>
> I think it probably works due to the other conditions?

The initial conditions are that:
cb->args[0..2] == 0.

So on the first iteration we set filter_wiphy == -1 and check the filter 
attributes.  If set, we modify filter_wiphy accordingly.

Even if filter_wiphy is set to 0, the if statement should still never be 
entered afterwards since wp_start and if_start are incremented.

Is this what you're worried about? Do you see a fault in my logic?

Regards,
-Denis

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Arend Van Spriel]

On 11-8-2016 20:20, Denis Kenzior wrote:
> Hi Johannes,
> 
>>> Speaking of indentation, can you point me to a doc of the rules I
>>> should follow?
>>
>> You've seen Documentation/CodingStyle?
> 
> Of course.  But that one doesn't discuss that you want your function
> parameters to be aligned to the opening '('.  Is there a dialect
> document specific to linux-wireless?

You can find it in checkpatch.pl [1] :-p

Regards,
Arend

[1] http://lxr.free-electrons.com/source/scripts/checkpatch.pl#L2855

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Denis Kenzior]

Hi Arend,

 >
> You can find it in checkpatch.pl [1] :-p
>

Aha!  Will use that one next time.  Thanks.

Regards,
-Denis

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Arend Van Spriel]

On 11-8-2016 21:05, Denis Kenzior wrote:
> Hi Arend,
> 
>>
>> You can find it in checkpatch.pl [1] :-p
>>
> 
> Aha!  Will use that one next time.  Thanks.

There is a '--strict' command line option. Not sure if this indentation
one false into that category.

Regards,
Arend

> Regards,
> -Denis

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Johannes Berg]

On Thu, 2016-08-11 at 13:20 -0500, Denis Kenzior wrote:
> Hi Johannes,
> 
>  >> Speaking of indentation, can you point me to a doc of the rules I
> > 
> > > 
> > > should follow?
> > 
> > You've seen Documentation/CodingStyle?
> 
> Of course.  But that one doesn't discuss that you want your function 
> parameters to be aligned to the opening '('.  Is there a dialect 
> document specific to linux-wireless?

Sorry. I don't think this is specific to our part of the tree, and I'm
surprised it's not in there. But I see Arend also pointed you to
checkpatch.pl, which, I might add, you shouldn't always take as
authoritative since sometimes "fixing" things for it makes the code
look worse.

> The initial conditions are that:
> cb->args[0..2] == 0.
> 
> So on the first iteration we set filter_wiphy == -1 and check the
> filter attributes.  If set, we modify filter_wiphy accordingly.
> 
> Even if filter_wiphy is set to 0, the if statement should still never
> be entered afterwards since wp_start and if_start are incremented.
> 
> Is this what you're worried about? Do you see a fault in my logic?

No, I don't see a fault in the logic. I just think it's misleading. You
make the code look like it relies on filter_wiphy != 0, but then you go
and treat filter_wiphy==0 as a valid case.

In other places, like nl80211_prepare_wdev_dump(), we add 1 to the
wiphy and subtract it again later to avoid exactly this. Perhaps you
could do the same, and rely only on filter_wiphy instead of really
relying only on wp_start/if_start.

johannes

Re: [RESEND PATCH] nl80211: Allow GET_INTERFACE dumps to be filtered

[Denis Kenzior]

Hi Johannes,

 >
> No, I don't see a fault in the logic. I just think it's misleading. You
> make the code look like it relies on filter_wiphy != 0, but then you go
> and treat filter_wiphy==0 as a valid case.

It relies on a sentry condition with all 3 variables being zero, not 
just filter_wiphy.  The original patch is about the most non-invasive 
one I can come up with.  Maybe a comment will alleviate any concerns? 
Anyway, if you feel this is misleading, fair enough.

>
> In other places, like nl80211_prepare_wdev_dump(), we add 1 to the
> wiphy and subtract it again later to avoid exactly this. Perhaps you
> could do the same, and rely only on filter_wiphy instead of really
> relying only on wp_start/if_start.

Having looked at that particular piece of code, I ran away scared with 
the conclusion that the cure is probably much worse than the disease :)

I posted a slightly different solution in v2.  It is a bit more 
invasive, but is more explicit in what is going on.  I ran sanity checks 
on it and it works as expected.

Regards,
-Denis