| Message ID | 20160226175925.GA9331@w1.fi (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
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
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..).
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
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
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 --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;