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

[João Paulo Rechi Vita]

Hello Johannes,

On 1 March 2016 at 11:15, João Paulo Rechi Vita  wrote:
> On 1 March 2016 at 08:43, Johannes Berg  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

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

[João Paulo Rechi Vita]

On 1 March 2016 at 08:43, Johannes Berg  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 = '@I'
>
> [...]
>
> 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

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

[Johannes Berg]

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 = '@I'

[...]

    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

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

[Jouni Malinen]

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..).
 
-- 
Jouni MalinenPGP id EFC895FA

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

[João Paulo Rechi Vita]

Hello Jouni,

On 26 February 2016 at 12:59, Jouni Malinen  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, _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, _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, _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, _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

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

[Jouni Malinen]

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, _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, _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, _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, _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;
 
-- 
Jouni MalinenPGP id EFC895FA

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

[João Paulo Rechi Vita]

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.

Signed-off-by: João Paulo Rechi Vita 
---
 net/rfkill/core.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 50b538b..04d7fa1 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -1185,6 +1185,7 @@ static ssize_t rfkill_fop_write(struct file *file, const 
char __user *buf,
 {
struct rfkill *rfkill;
struct rfkill_event ev;
+   int ret = 0;
 
/* we don't need the 'hard' variable but accept it */
if (count < RFKILL_EVENT_SIZE_V1 - 1)
@@ -1199,29 +1200,32 @@ static ssize_t rfkill_fop_write(struct file *file, 
const char __user *buf,
if (copy_from_user(, buf, count))
return -EFAULT;
 
-   if (ev.op != RFKILL_OP_CHANGE && ev.op != RFKILL_OP_CHANGE_ALL)
-   return -EINVAL;
-
if (ev.type >= NUM_RFKILL_TYPES)
return -EINVAL;
 
mutex_lock(_global_mutex);
 
-   if (ev.op == RFKILL_OP_CHANGE_ALL)
+   switch (ev.op) {
+   case RFKILL_OP_CHANGE_ALL:
rfkill_update_global_state(ev.type, ev.soft);
-
-   list_for_each_entry(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;
-
-   rfkill_set_block(rfkill, ev.soft);
+   list_for_each_entry(rfkill, _list, node)
+   if (rfkill->type == ev.type ||
+   ev.type == RFKILL_TYPE_ALL)
+   rfkill_set_block(rfkill, ev.soft);
+   break;
+   case RFKILL_OP_CHANGE:
+   list_for_each_entry(rfkill, _list, node)
+   if (rfkill->idx == ev.idx && rfkill->type == ev.type)
+   rfkill_set_block(rfkill, ev.soft);
+   break;
+   default:
+   ret = -EINVAL;
+   break;
}
+
mutex_unlock(_global_mutex);
 
-   return count;
+   return ret ? ret : count;
 }
 
 static int rfkill_fop_release(struct inode *inode, struct file *file)
-- 
2.5.0