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 <[email protected]>
---
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
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
Hi Arend,
>
> You can find it in checkpatch.pl [1] :-p
>
Aha! Will use that one next time. Thanks.
Regards,
-Denis
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
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
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
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
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
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
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