[PATCH] scan: don't send the same scan request twice

[PATCH] scan: don't send the same scan request twice

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

If start_scan_next_request() is called while a scan request
(NL80211_CMD_GET_SCAN) is still running, the same scan request will be
sent again by mistake. Add a check in the function to avoid sending a
request if one is already in progress.

This also fixes a crash that occurs if the following conditions are met:
  - the duplicated request is the only request in the scan request
    queue, and
  - both scan requests fail with an error not EBUSY.

In this case, the first callback to scan_request_triggered() will delete
the request from the scan request queue. The second callback will find
an empty queue and consequently pass a NULL scan_request pointer to
scan_request_failed(), causing a segmentation fault.
---
 src/scan.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/scan.c b/src/scan.c
index 718f7497..106fa81c 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -839,6 +839,9 @@ static bool start_next_scan_request(struct scan_context *sc)
 	if (sc->state != SCAN_STATE_NOT_RUNNING)
 		return true;
 
+	if (sc->start_cmd_id)
+		return true;
+
 	while (sr) {
 		if (!scan_request_send_trigger(sc, sr))
 			return true;
-- 
2.26.2

Re: [PATCH] scan: don't send the same scan request twice

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

Hi Alvin,
(resending this to the list)

On Fri, 29 May 2020 at 15:57, Alvin Šipraga <alsi@bang-olufsen.dk> wrote:
>
> If start_scan_next_request() is called while a scan request
> (NL80211_CMD_GET_SCAN) is still running, the same scan request will be
> sent again by mistake. Add a check in the function to avoid sending a
> request if one is already in progress.

Do you know if start_scan_next_request() was called from scan_resume
in your case?  In all other callers we can be sure start_cmd_id is
already 0, so scan_resume may need to have these checks added instead.
I think it should check that !sc->start_cmd_id && !sc->triggered &&
!sc->get_scan_cmd_id because the scan_resume() call can come in during
any of those phases.

BTW you mention NL80211_CMD_GET_SCAN above, I think you meant
NL80211_CMD_TRIGGER_SCAN.

I also wonder if scan_common() should check sc->suspended like
start_scan_next_request() does.

Best regards

Re: [PATCH] scan: don't send the same scan request twice

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

Hi Andrew,

On 5/31/20 12:26 AM, Andrew Zaborowski wrote:
> Hi Alvin,
> (resending this to the list)
> 
> On Fri, 29 May 2020 at 15:57, Alvin Šipraga <alsi@bang-olufsen.dk> wrote:
>>
>> If start_scan_next_request() is called while a scan request
>> (NL80211_CMD_GET_SCAN) is still running, the same scan request will be
>> sent again by mistake. Add a check in the function to avoid sending a
>> request if one is already in progress.
> 
> Do you know if start_scan_next_request() was called from scan_resume
> in your case?  In all other callers we can be sure start_cmd_id is
> already 0, so scan_resume may need to have these checks added instead.
> I think it should check that !sc->start_cmd_id && !sc->triggered &&
> !sc->get_scan_cmd_id because the scan_resume() call can come in during
> any of those phases.

Yes, in my case it was getting called from scan_resume(). I agree that
it's the only point where start_scan_next_request() might get called
while start_cmd_id != 0, so what you suggest would also fix the crash I
referred to.

But it seems to me that scan_resume() should not really concern itself
with the internals of the scan request dispatching. Given that
start_next_scan_request() already returns true early if sc->suspended or
if sc->state != SCAN_STATE_NOT_RUNNING, it seems like an optimistic "try
to send the next scan request if it is possible" function. Therefore I
put the extra check (sc->start_cmd_id != 0) there in the same vein.

What do you think?

> 
> BTW you mention NL80211_CMD_GET_SCAN above, I think you meant
> NL80211_CMD_TRIGGER_SCAN.

Yes, you are right. I will fix this when I send a v2 patch.

> 
> I also wonder if scan_common() should check sc->suspended like
> start_scan_next_request() does.

I guess it should, no?

Perhaps rather than having duplicate checks for the same stuff in both
scan_common() and start_next_scan_request(), we could have scan_common()
enqueue its scan request and just call directly into
start_next_scan_request()? Then we can put all the relevant checks in
one place (start_next_scan_request()), namely: !sc->suspended &&
sc->state != SCAN_STATE_NOT_RUNNING && !sc->start_cmd_id &&
!sc->get_scan_cmd_id.

The only problem with having scan_common() use start_next_scan_request()
is that, currently, scan_common() deliberately removes the destroy
callback from the scan request if scan_request_send_trigger() returns an
error:

    if (!scan_request_send_trigger(sc, sr))
        goto done;

    sr->destroy = NULL; /* Don't call destroy when returning error */
    scan_request_free(sr);

I'm not quite sure what the reason for this is, but maybe you can comment?

If you like the idea of moving most of the pre-scan-trigger checks into
one place, then I can prepare a patch for you to see exactly what I
mean. But if you think it's overkill then I will just resend a v2 of
this minimal change to address the specific problem (duplicate scan
request & segfault).

Kind regards,
Alvin

Re: [PATCH] scan: don't send the same scan request twice

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3460 bytes --]

On Mon, 1 Jun 2020 at 13:32, Alvin Šipraga <alsi@bang-olufsen.dk> wrote:
> On 5/31/20 12:26 AM, Andrew Zaborowski wrote:
> > Do you know if start_scan_next_request() was called from scan_resume
> > in your case?  In all other callers we can be sure start_cmd_id is
> > already 0, so scan_resume may need to have these checks added instead.
> > I think it should check that !sc->start_cmd_id && !sc->triggered &&
> > !sc->get_scan_cmd_id because the scan_resume() call can come in during
> > any of those phases.
>
> Yes, in my case it was getting called from scan_resume(). I agree that
> it's the only point where start_scan_next_request() might get called
> while start_cmd_id != 0, so what you suggest would also fix the crash I
> referred to.
>
> But it seems to me that scan_resume() should not really concern itself
> with the internals of the scan request dispatching. Given that
> start_next_scan_request() already returns true early if sc->suspended or
> if sc->state != SCAN_STATE_NOT_RUNNING, it seems like an optimistic "try
> to send the next scan request if it is possible" function. Therefore I
> put the extra check (sc->start_cmd_id != 0) there in the same vein.
>
> What do you think?

It would probably work.  The downside is that we're possibly
re-checking conditions that we already know are fulfilled, the upside
is possibly readability.

You'd then need to change the commit message to say it's refactoring
start_next_scan_request.

>
> >
> > BTW you mention NL80211_CMD_GET_SCAN above, I think you meant
> > NL80211_CMD_TRIGGER_SCAN.
>
> Yes, you are right. I will fix this when I send a v2 patch.
>
> >
> > I also wonder if scan_common() should check sc->suspended like
> > start_scan_next_request() does.
>
> I guess it should, no?
>
> Perhaps rather than having duplicate checks for the same stuff in both
> scan_common() and start_next_scan_request(), we could have scan_common()
> enqueue its scan request and just call directly into
> start_next_scan_request()? Then we can put all the relevant checks in
> one place (start_next_scan_request()), namely: !sc->suspended &&
> sc->state != SCAN_STATE_NOT_RUNNING && !sc->start_cmd_id &&
> !sc->get_scan_cmd_id.
>
> The only problem with having scan_common() use start_next_scan_request()
> is that, currently, scan_common() deliberately removes the destroy
> callback from the scan request if scan_request_send_trigger() returns an
> error:
>
>     if (!scan_request_send_trigger(sc, sr))
>         goto done;
>
>     sr->destroy = NULL; /* Don't call destroy when returning error */
>     scan_request_free(sr);
>
> I'm not quite sure what the reason for this is, but maybe you can comment?

The reason is that the scan_passive, scan_active, etc. entry points
are not assumed to call the destroy callback synchronously. When they
return an error it's assumed that none of the resources passed as
parameters were used and no callbacks happened, i.e. funcions have no
side effects on errors, afaik it's just a convention.

>
> If you like the idea of moving most of the pre-scan-trigger checks into
> one place, then I can prepare a patch for you to see exactly what I
> mean. But if you think it's overkill then I will just resend a v2 of
> this minimal change to address the specific problem (duplicate scan
> request & segfault).

You can try it and we'll see what Denis thinks about it.

Best regards

Re: [PATCH] scan: don't send the same scan request twice

[Alvin =?unknown-8bit?q?=C5=A0ipraga?=]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On 6/1/20 3:20 PM, Andrew Zaborowski wrote> It would probably work.  The
downside is that we're possibly
> re-checking conditions that we already know are fulfilled, the upside
> is possibly readability.
> 
> You'd then need to change the commit message to say it's refactoring
> start_next_scan_request.

I will send a v2 patch with an updated commit message addressing your
comments:
 - NL80211_CMD_GET_SCAN -> NL80211_CMD_TRIGGER_SCAN
 - mention that start_next_scan_request() is being refactored

>> I'm not quite sure what the reason for this is, but maybe you can comment?
> 
> The reason is that the scan_passive, scan_active, etc. entry points
> are not assumed to call the destroy callback synchronously. When they
> return an error it's assumed that none of the resources passed as
> parameters were used and no callbacks happened, i.e. funcions have no
> side effects on errors, afaik it's just a convention.

OK, I understand. Considering that (reasonable) convention, I guess it's
not a good idea to have scan_common() call into
start_next_scan_request(). Please disregard my suggestion.

Please check out the v2 patch I have posted. I also added a second patch
to check for sc->suspended in scan_common(). You can take it as well if
you think it's needed.

Kind regards,
Alvin