diff mbox

[PATCHv2,08/10] rfkill: Use switch to demux userspace operations

Message ID 20160226175925.GA9331@w1.fi
State Not Applicable
Delegated to: Johannes Berg
Headers show

Commit Message

Jouni Malinen Feb. 26, 2016, 5:59 p.m. UTC
On Mon, Feb 22, 2016 at 11:36:39AM -0500, João Paulo Rechi Vita wrote:
> Using a switch to handle different ev.op values in rfkill_fop_write()
> makes the code easier to extend, as out-of-range values can always be
> handled by the default case.

This breaks rfkill.. There are automated test scripts for testing this
area (and most of Wi-Fi for that matter. It would be nice if these were
used for changes before they get contributed upstream..

http://buildbot.w1.fi/hwsim/

This specific commit broke all the rfkill_* test cases because of
following:

> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> @@ -1199,29 +1200,32 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
> -	list_for_each_entry(rfkill, &rfkill_list, node) {
> -		if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
> -			continue;
> -
> -		if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
> -			continue;

Note that RFKILL_TYPE_ALL here..

> +		list_for_each_entry(rfkill, &rfkill_list, node)
> +			if (rfkill->type == ev.type ||
> +			    ev.type == RFKILL_TYPE_ALL)
> +				rfkill_set_block(rfkill, ev.soft);

It was included for RFKILL_OP_CHANGE_ALL.

> +	case RFKILL_OP_CHANGE:
> +		list_for_each_entry(rfkill, &rfkill_list, node)
> +			if (rfkill->idx == ev.idx && rfkill->type == ev.type)
> +				rfkill_set_block(rfkill, ev.soft);

but not for RFKILL_OP_CHANGE..

This needs following to work:

Comments

João Paulo Rechi Vita Feb. 29, 2016, 10:30 p.m. UTC | #1
Hello Jouni,

On 26 February 2016 at 12:59, Jouni Malinen <j@w1.fi> wrote:
> On Mon, Feb 22, 2016 at 11:36:39AM -0500, João Paulo Rechi Vita wrote:
>> Using a switch to handle different ev.op values in rfkill_fop_write()
>> makes the code easier to extend, as out-of-range values can always be
>> handled by the default case.
>
> This breaks rfkill.. There are automated test scripts for testing this
> area (and most of Wi-Fi for that matter. It would be nice if these were
> used for changes before they get contributed upstream..
>
> http://buildbot.w1.fi/hwsim/
>

Thanks for pointing that out, I haven't heard of this tool before.
I'll give it a try before my next submission.

> This specific commit broke all the rfkill_* test cases because of
> following:
>
>> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
>> @@ -1199,29 +1200,32 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>> -     list_for_each_entry(rfkill, &rfkill_list, node) {
>> -             if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
>> -                     continue;
>> -
>> -             if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
>> -                     continue;
>
> Note that RFKILL_TYPE_ALL here..
>
>> +             list_for_each_entry(rfkill, &rfkill_list, node)
>> +                     if (rfkill->type == ev.type ||
>> +                         ev.type == RFKILL_TYPE_ALL)
>> +                             rfkill_set_block(rfkill, ev.soft);
>
> It was included for RFKILL_OP_CHANGE_ALL.
>
>> +     case RFKILL_OP_CHANGE:
>> +             list_for_each_entry(rfkill, &rfkill_list, node)
>> +                     if (rfkill->idx == ev.idx && rfkill->type == ev.type)
>> +                             rfkill_set_block(rfkill, ev.soft);
>
> but not for RFKILL_OP_CHANGE..
>
> This needs following to work:
>
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index 59ff92d..c4bbd19 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -1239,7 +1239,9 @@ static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
>                 break;
>         case RFKILL_OP_CHANGE:
>                 list_for_each_entry(rfkill, &rfkill_list, node)
> -                       if (rfkill->idx == ev.idx && rfkill->type == ev.type)
> +                       if (rfkill->idx == ev.idx &&
> +                           (rfkill->type == ev.type ||
> +                            ev.type == RFKILL_TYPE_ALL))
>                                 rfkill_set_block(rfkill, ev.soft);
>                 ret = 0;
>                 break;
>

I agree there is a difference in the logic here, thanks for taking the
time to point it out so clearly, and sorry for missing this. But AFAIU
userspace should not call RFKILL_OP_CHANGE with ev.type ==
RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
block/unblock one RFKill switch, and it is not possible to create a
RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
return NULL).

I tried to look into the source code of the test suite you pointed,
but couldn't easily figure out how it ends up with that combination.
Could you please explain (or point me in the code) how is that a valid
operation? If I'm not missing anything, we should probably return
EINVAL in this case.

Regards,

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jouni Malinen Feb. 29, 2016, 10:39 p.m. UTC | #2
On Mon, Feb 29, 2016 at 05:30:20PM -0500, João Paulo Rechi Vita wrote:

> I agree there is a difference in the logic here, thanks for taking the
> time to point it out so clearly, and sorry for missing this. But AFAIU
> userspace should not call RFKILL_OP_CHANGE with ev.type ==
> RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
> block/unblock one RFKill switch, and it is not possible to create a
> RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
> return NULL).

Interesting. Maybe Johannes can comment on that part since I think he
wrote the code that interacts with kernel for the rfkill test cases.

> I tried to look into the source code of the test suite you pointed,
> but couldn't easily figure out how it ends up with that combination.
> Could you please explain (or point me in the code) how is that a valid
> operation? If I'm not missing anything, we should probably return
> EINVAL in this case.

These specific failures were shown for the test cases in this file:
http://w1.fi/cgit/hostap/tree/tests/hwsim/test_rfkill.py

The interaction with kernel is done using this code:
http://w1.fi/cgit/hostap/tree/tests/hwsim/rfkill.py

It does indeed look like TYPE_ALL is used here (the block() and
unblock() implementations). If this is incorrect, we can certainly
change the script since I'd assume this is not used for anything else
than the hwsim test cases (or well who knows, it is available out there,
so if someone needs python code to do rfkill operations..).
Johannes Berg March 1, 2016, 1:43 p.m. UTC | #3
On Tue, 2016-03-01 at 00:39 +0200, Jouni Malinen wrote:

> > I agree there is a difference in the logic here,

Gah. I thought I'd reviewed the logic and made sure there's no
difference ... :)

> >  thanks for taking the
> > time to point it out so clearly, and sorry for missing this. But AFAIU
> > userspace should not call RFKILL_OP_CHANGE with ev.type ==
> > RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
> > block/unblock one RFKill switch, and it is not possible to create a
> > RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
> > return NULL).

> Interesting. Maybe Johannes can comment on that part since I think he
> wrote the code that interacts with kernel for the rfkill test cases.

So first of all, it seems that this argument is invalid since we can't break the ABI/API here; although perhaps if it's only a test case ...

Oh. It took me a while, but I see now. The original intent (I think)
was that with RFKILL_OP_CHANGE, the type would be ignored entirely. It
seems that the (my) original intent wouldn't have been to force
userspace to specify *both* the index and the type, but instead do

OP_CHANGE_ALL -> use type (possibly TYPE_ALL, ignoring idx)
OP_CHANGE     -> use idx (ignoring type)


The original code implemented it as follows:

                if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
                        continue;

-> check the idx only for OP_CHANGE

                if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
                        continue;

-> check the type, allowing _ALL

Now, all userspace that I found sets the ev.type field to TYPE_ALL all
the time; and it had to given these checks.

e.g. from rfkill.py:

# idx, type, op, soft, hard
_event_struct = '@IBBBB'

[...]

    def block(self):
        rfk = open('/dev/rfkill', 'w')
        s = struct.pack(_event_struct, self.idx, TYPE_ALL, _OP_CHANGE, 1, 0)
        rfk.write(s)
        rfk.close()


This check, originally, probably should've been

                if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL &&
                    ev.op != RFKILL_OP_CHANGE)
                        continue;

to ignore the type entirely.

I'm fine with Jouni's change, preserving the original behaviour of
requiring TYPE_ALL or the correct type, but I'm tempted to simply
remove the type check entirely.

Thoughts?

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
João Paulo Rechi Vita March 1, 2016, 4:15 p.m. UTC | #4
On 1 March 2016 at 08:43, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2016-03-01 at 00:39 +0200, Jouni Malinen wrote:
>
>> > I agree there is a difference in the logic here,
>
> Gah. I thought I'd reviewed the logic and made sure there's no
> difference ... :)
>
>> >  thanks for taking the
>> > time to point it out so clearly, and sorry for missing this. But AFAIU
>> > userspace should not call RFKILL_OP_CHANGE with ev.type ==
>> > RFKILL_TYPE_ALL, as RFKILL_OP_CHANGE is intended to be used to
>> > block/unblock one RFKill switch, and it is not possible to create a
>> > RFKill switch with type == RFKILL_TYPE_ALL (rfkill_alloc() would
>> > return NULL).
>
>> Interesting. Maybe Johannes can comment on that part since I think he
>> wrote the code that interacts with kernel for the rfkill test cases.
>
> So first of all, it seems that this argument is invalid since we can't break the ABI/API here; although perhaps if it's only a test case ...
>

Yep, that's an important point (not breaking the API/ABI).

> Oh. It took me a while, but I see now. The original intent (I think)
> was that with RFKILL_OP_CHANGE, the type would be ignored entirely. It
> seems that the (my) original intent wouldn't have been to force
> userspace to specify *both* the index and the type, but instead do
>
> OP_CHANGE_ALL -> use type (possibly TYPE_ALL, ignoring idx)
> OP_CHANGE     -> use idx (ignoring type)
>
>
> The original code implemented it as follows:
>
>                 if (rfkill->idx != ev.idx && ev.op != RFKILL_OP_CHANGE_ALL)
>                         continue;
>
> -> check the idx only for OP_CHANGE
>
>                 if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL)
>                         continue;
>
> -> check the type, allowing _ALL
>
> Now, all userspace that I found sets the ev.type field to TYPE_ALL all
> the time; and it had to given these checks.
>
> e.g. from rfkill.py:
>
> # idx, type, op, soft, hard
> _event_struct = '@IBBBB'
>
> [...]
>
>     def block(self):
>         rfk = open('/dev/rfkill', 'w')
>         s = struct.pack(_event_struct, self.idx, TYPE_ALL, _OP_CHANGE, 1, 0)
>         rfk.write(s)
>         rfk.close()
>
>
> This check, originally, probably should've been
>
>                 if (rfkill->type != ev.type && ev.type != RFKILL_TYPE_ALL &&
>                     ev.op != RFKILL_OP_CHANGE)
>                         continue;
>
> to ignore the type entirely.
>
> I'm fine with Jouni's change, preserving the original behaviour of
> requiring TYPE_ALL or the correct type, but I'm tempted to simply
> remove the type check entirely.
>
> Thoughts?
>

I think this patch should keep the original logic, as this is supposed
to be a refactor only. If we decide to remove the check, we should to
it in a separate patch, to make it clear for someone looking at the
history later.

I'm fine with removing the type check (in a separate patch), but I
don't see much gain in doing so.

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
João Paulo Rechi Vita March 8, 2016, 2:01 p.m. UTC | #5
Hello Johannes,

On 1 March 2016 at 11:15, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
> On 1 March 2016 at 08:43, Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>> I'm fine with Jouni's change, preserving the original behaviour of
>> requiring TYPE_ALL or the correct type, but I'm tempted to simply
>> remove the type check entirely.
>>
>> Thoughts?
>>
>
> I think this patch should keep the original logic, as this is supposed
> to be a refactor only. If we decide to remove the check, we should to
> it in a separate patch, to make it clear for someone looking at the
> history later.
>
> I'm fine with removing the type check (in a separate patch), but I
> don't see much gain in doing so.
>

I just saw you picked this patch with Jouni's fix, thanks!

--
João Paulo Rechi Vita
http://about.me/jprvita
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 59ff92d..c4bbd19 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -1239,7 +1239,9 @@  static ssize_t rfkill_fop_write(struct file *file, const char __user *buf,
 		break;
 	case RFKILL_OP_CHANGE:
 		list_for_each_entry(rfkill, &rfkill_list, node)
-			if (rfkill->idx == ev.idx && rfkill->type == ev.type)
+			if (rfkill->idx == ev.idx &&
+			    (rfkill->type == ev.type ||
+			     ev.type == RFKILL_TYPE_ALL))
 				rfkill_set_block(rfkill, ev.soft);
 		ret = 0;
 		break;