* Re: [PATCH] service: Complete only after user connection retries
[not found] ` <20210327124254.iorhou27y3u4pwih@beryllium.lan>
@ 2021-06-15 11:09 ` John Keeping
2021-06-28 7:58 ` Daniel Wagner
0 siblings, 1 reply; 5+ messages in thread
From: John Keeping @ 2021-06-15 11:09 UTC (permalink / raw)
To: Daniel Wagner, VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman
Hi,
On Sat, 27 Mar 2021 13:42:54 +0100
Daniel Wagner <wagi@monom.org> wrote:
> On Thu, Mar 11, 2021 at 01:04:11PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > When a user connection fails, let the report_error_cb manage
> > exclusively service update, after all retries.
>
> Patch applied.
This patch seems to have totally broken connection attempts where the
wrong passphrase is entered:
connmanctl> connect wifi_b0f1ecffb46f_7073475462456b44_managed_psk
Agent RequestInput wifi_b0f1ecffb46f_7073475462456b44_managed_psk
Passphrase = [ Type=psk, Requirement=mandatory ]
Passphrase? wrong-passphrase
Agent ReportError wifi_b0f1ecffb46f_7073475462456b44_managed_psk
invalid-key
Agent request cancelled by ConnMan
connmanctl> connect wifi_b0f1ecffb46f_7073475462456b44_managed_psk
Error /net/connman/service/wifi_b0f1ecffb46f_7073475462456b44_managed_psk: In progress
This happens because service->pending is never cleared because
service_cleared() is bypassed in this hunk:
@@ -6064,6 +6065,7 @@ static int service_indicate_state(struct connman_service *service)
report_error_cb,
get_dbus_sender(service),
NULL);
+ goto notifier;
}
service_complete(service);
break;
It looks like something is missing because reporting the error only
after all retries makes sense, but after this patch the original connect
request doesn't get a response at all and any future attempt to connect
fails.
I'm not familiar enough with the code flow to suggest a follow-up
change, but does the above explanation make sense? Is there an easy way
to ensure that the pending state is cleared and we reply to the original
message?
Thanks,
John
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] service: Complete only after user connection retries
2021-06-15 11:09 ` [PATCH] service: Complete only after user connection retries John Keeping
@ 2021-06-28 7:58 ` Daniel Wagner
2021-06-28 15:04 ` John Keeping
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Wagner @ 2021-06-28 7:58 UTC (permalink / raw)
To: John Keeping; +Cc: VAUTRIN Emmanuel (Canal Plus Prestataire), connman
Hi John,
Sorry for the long delay.
On Tue, Jun 15, 2021 at 12:09:30PM +0100, John Keeping wrote:
> Hi,
>
> On Sat, 27 Mar 2021 13:42:54 +0100
> Daniel Wagner <wagi@monom.org> wrote:
> > On Thu, Mar 11, 2021 at 01:04:11PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > > When a user connection fails, let the report_error_cb manage
> > > exclusively service update, after all retries.
> >
> > Patch applied.
>
> This patch seems to have totally broken connection attempts where the
> wrong passphrase is entered:
>
> connmanctl> connect wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> Agent RequestInput wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> Passphrase = [ Type=psk, Requirement=mandatory ]
> Passphrase? wrong-passphrase
> Agent ReportError wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> invalid-key
> Agent request cancelled by ConnMan
> connmanctl> connect wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> Error /net/connman/service/wifi_b0f1ecffb46f_7073475462456b44_managed_psk: In progress
>
> This happens because service->pending is never cleared because
> service_cleared() is bypassed in this hunk:
>
> @@ -6064,6 +6065,7 @@ static int service_indicate_state(struct connman_service *service)
> report_error_cb,
> get_dbus_sender(service),
> NULL);
> + goto notifier;
> }
> service_complete(service);
> break;
>
> It looks like something is missing because reporting the error only
> after all retries makes sense, but after this patch the original connect
> request doesn't get a response at all and any future attempt to connect
> fails.
>
> I'm not familiar enough with the code flow to suggest a follow-up
> change, but does the above explanation make sense? Is there an easy way
> to ensure that the pending state is cleared and we reply to the original
> message?
I've tried to reproduce it. With iwd, we never go this path as iwd tells
us invalid key directly from the connect attempt.
In case of wpa_supplicant we go through this path, meaning
report_error_cb gets called with either retry=1 or retry=0:
connmanctl> connect wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
Passphrase = [ Type=psk, Requirement=mandatory ]
Passphrase? asdf
Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
invalid-key
connmanctl> Retry (yes/no)? yes
Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
Passphrase = [ Type=psk, Requirement=mandatory ]
Passphrase? asdf
Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
invalid-key
connmanctl> Retry (yes/no)? no
Error /net/connman/service/wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk: Input/output error
connmanctl> connect wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
Passphrase = [ Type=psk, Requirement=mandatory ]
Passphrase? asdf
Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
invalid-key
connmanctl> Retry (yes/no)? yes
Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
Passphrase = [ Type=psk, Requirement=mandatory ]
Passphrase? asdf
Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
invalid-key
connmanctl> Retry (yes/no)? yes
Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
Passphrase = [ Type=psk, Requirement=mandatory ]
Passphrase? correct-password
Connected wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
So the only way I can see why the pending message is not released is a
missing call to report_error_cb() or the final call is
report_error_cb(retry=1).
Could you try to add some debug printfs and report back?
Thanks,
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] service: Complete only after user connection retries
2021-06-28 7:58 ` Daniel Wagner
@ 2021-06-28 15:04 ` John Keeping
2021-06-28 19:54 ` Daniel Wagner
0 siblings, 1 reply; 5+ messages in thread
From: John Keeping @ 2021-06-28 15:04 UTC (permalink / raw)
To: Daniel Wagner; +Cc: VAUTRIN Emmanuel (Canal Plus Prestataire), connman
Hi Daniel,
On Mon, 28 Jun 2021 09:58:35 +0200
Daniel Wagner <wagi@monom.org> wrote:
> Sorry for the long delay.
>
> On Tue, Jun 15, 2021 at 12:09:30PM +0100, John Keeping wrote:
> > Hi,
> >
> > On Sat, 27 Mar 2021 13:42:54 +0100
> > Daniel Wagner <wagi@monom.org> wrote:
> > > On Thu, Mar 11, 2021 at 01:04:11PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > > > When a user connection fails, let the report_error_cb manage
> > > > exclusively service update, after all retries.
> > >
> > > Patch applied.
> >
> > This patch seems to have totally broken connection attempts where the
> > wrong passphrase is entered:
> >
> > connmanctl> connect wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> > Agent RequestInput wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> > Passphrase = [ Type=psk, Requirement=mandatory ]
> > Passphrase? wrong-passphrase
> > Agent ReportError wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> > invalid-key
> > Agent request cancelled by ConnMan
> > connmanctl> connect wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> > Error /net/connman/service/wifi_b0f1ecffb46f_7073475462456b44_managed_psk: In progress
> >
> > This happens because service->pending is never cleared because
> > service_cleared() is bypassed in this hunk:
> >
> > @@ -6064,6 +6065,7 @@ static int service_indicate_state(struct connman_service *service)
> > report_error_cb,
> > get_dbus_sender(service),
> > NULL);
> > + goto notifier;
> > }
> > service_complete(service);
> > break;
> >
> > It looks like something is missing because reporting the error only
> > after all retries makes sense, but after this patch the original connect
> > request doesn't get a response at all and any future attempt to connect
> > fails.
> >
> > I'm not familiar enough with the code flow to suggest a follow-up
> > change, but does the above explanation make sense? Is there an easy way
> > to ensure that the pending state is cleared and we reply to the original
> > message?
>
> I've tried to reproduce it. With iwd, we never go this path as iwd tells
> us invalid key directly from the connect attempt.
>
> In case of wpa_supplicant we go through this path, meaning
> report_error_cb gets called with either retry=1 or retry=0:
>
> connmanctl> connect wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> Passphrase = [ Type=psk, Requirement=mandatory ]
> Passphrase? asdf
> Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> invalid-key
> connmanctl> Retry (yes/no)? yes
> Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> Passphrase = [ Type=psk, Requirement=mandatory ]
> Passphrase? asdf
> Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> invalid-key
> connmanctl> Retry (yes/no)? no
> Error /net/connman/service/wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk: Input/output error
> connmanctl> connect wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> Passphrase = [ Type=psk, Requirement=mandatory ]
> Passphrase? asdf
> Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> invalid-key
> connmanctl> Retry (yes/no)? yes
> Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> Passphrase = [ Type=psk, Requirement=mandatory ]
> Passphrase? asdf
> Agent ReportError wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> invalid-key
> connmanctl> Retry (yes/no)? yes
> Agent RequestInput wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
> Passphrase = [ Type=psk, Requirement=mandatory ]
> Passphrase? correct-password
> Connected wifi_001e58ab5802_6f776c2073747265746368696e672074696d65_managed_psk
>
>
> So the only way I can see why the pending message is not released is a
> missing call to report_error_cb() or the final call is
> report_error_cb(retry=1).
>
> Could you try to add some debug printfs and report back?
It looks like the length of the invalid passphrase matters here. If I
use a short passphrase like you have above then I reproduce that
behaviour, but if the wrong passphrase has at least 8 characters then it
enters the failure state.
I think __connman_service_check_passphrase() is rejecting the short
passphrase so it never even gets to wpa_supplicant.
In the failing case, I don't get the chance to choose to retry. Instead
the output looks like:
Passphrase? asdfgghjgj
Agent ReportError wifi_b0f1ecffb46f_7073475462456b44_managed_psk
invalid-key
Agent request cancelled by ConnMan
Retry (yes/no)? connmanctl>
where the new prompt appears instantly. Inspecting the DBus traffic, I
see that connman calls Agent.ReportError and then immediately calls
Agent.Cancel (the timestamp is less than a millisecond later, although
there is a call to fi.w1.fpa_supplicant1.Interface.Disconnect in
between).
I added extra logging in report_error_cb() and can confirm that this is
never called when this happens.
Regards,
John
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] service: Complete only after user connection retries
2021-06-28 15:04 ` John Keeping
@ 2021-06-28 19:54 ` Daniel Wagner
2021-06-29 7:35 ` Daniel Wagner
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Wagner @ 2021-06-28 19:54 UTC (permalink / raw)
To: John Keeping; +Cc: VAUTRIN Emmanuel (Canal Plus Prestataire), connman
Hi John,
On Mon, Jun 28, 2021 at 04:04:13PM +0100, John Keeping wrote:
> > Could you try to add some debug printfs and report back?
>
> It looks like the length of the invalid passphrase matters here. If I
> use a short passphrase like you have above then I reproduce that
> behaviour, but if the wrong passphrase has at least 8 characters then it
> enters the failure state.
D'oh! You are so right :)
> I think __connman_service_check_passphrase() is rejecting the short
> passphrase so it never even gets to wpa_supplicant.
>
> In the failing case, I don't get the chance to choose to retry. Instead
> the output looks like:
>
> Passphrase? asdfgghjgj
> Agent ReportError wifi_b0f1ecffb46f_7073475462456b44_managed_psk
> invalid-key
> Agent request cancelled by ConnMan
> Retry (yes/no)? connmanctl>
>
> where the new prompt appears instantly. Inspecting the DBus traffic, I
> see that connman calls Agent.ReportError and then immediately calls
> Agent.Cancel (the timestamp is less than a millisecond later, although
> there is a call to fi.w1.fpa_supplicant1.Interface.Disconnect in
> between).
>
> I added extra logging in report_error_cb() and can confirm that this is
> never called when this happens.
Alright, we are getting closer. Hmm, just an idea what could happen is
the agent state machine gets canceled (the supplicant is likely to say,
password is not working) and this it eats up the pending callback.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] service: Complete only after user connection retries
2021-06-28 19:54 ` Daniel Wagner
@ 2021-06-29 7:35 ` Daniel Wagner
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Wagner @ 2021-06-29 7:35 UTC (permalink / raw)
To: John Keeping; +Cc: VAUTRIN Emmanuel (Canal Plus Prestataire), connman
Hi John,
On Mon, Jun 28, 2021 at 09:54:11PM +0200, Daniel Wagner wrote:
> Alright, we are getting closer. Hmm, just an idea what could happen is
> the agent state machine gets canceled (the supplicant is likely to say,
> password is not working) and this it eats up the pending callback.
I think I found the place were lost the callback. I just send out the
patch. Please give it a go. If possible please also test some other use
cases. I am not sure if this change is not introduce another
regression...
Thanks,
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-29 7:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <PR1PR02MB4794EB6ADA4A23AA374EE58493909@PR1PR02MB4794.eurprd02.prod.outlook.com>
[not found] ` <20210327124254.iorhou27y3u4pwih@beryllium.lan>
2021-06-15 11:09 ` [PATCH] service: Complete only after user connection retries John Keeping
2021-06-28 7:58 ` Daniel Wagner
2021-06-28 15:04 ` John Keeping
2021-06-28 19:54 ` Daniel Wagner
2021-06-29 7:35 ` Daniel Wagner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).