Mailing List Archive

On Wed, Jul 4, 2012 at 3:13 AM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
> This allows persisted tun device to be cleaned up for reuse.
>
> This is the minimal change to reach the goal using atexit(),
> not sure it is the best way.

Thanks for the patch set.

I do not understand the reason for adding this patch.
Current code in vpnc already runs close_tunnel() before exit, at least
in all the cases I have tested. Your patch forces running
close_tunnel() again for a second time.
Cases of abrupt crash, e.g. segmentation fault, are not handled in
current code neither in your patch, but such cases should be addressed
by fixing directly the crash issue.

I expect you have found some specific case in current code that fails
to run close_tunnel().
Or, the case of persistent tun device is not well cleared by current
close_tunnel() and you need to run it twice. In this case the right
target would be fixing existing code or script.

Could you please report the specific use case that requires this patch?

If you run vpnc with "--debug 2" you will get message
S8 close_tunnel
right before close_tunnel() is executed by existing code.
This could help you to narrow the search.

Best Regards,
Antonio Borneo

>
> Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
> ---
> vpnc.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/vpnc.c b/vpnc.c
> index 6ab10eb..f72d8ec 100644
> --- a/vpnc.c
> +++ b/vpnc.c
> @@ -482,11 +482,23 @@ static void setup_tunnel(struct sa_block *s)
> }
> }
>
> +static struct sa_block *s_atexit_sa;
> +static void close_tunnel(struct sa_block *s);
> +static void atexit_close(void)
> +{
> + if (s_atexit_sa != NULL) {
> + close_tunnel(s_atexit_sa);
> + s_atexit_sa = NULL;
> + }
> +}
> +
> static void config_tunnel(struct sa_block *s)
> {
> setenv("VPNGATEWAY", inet_ntoa(s->dst), 1);
> setenv("reason", "connect", 1);
> system(config[CONFIG_SCRIPT]);
> + s_atexit_sa = s;
> + atexit(atexit_close);
> }
>
> static void close_tunnel(struct sa_block *s)
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Wed, Jul 4, 2012 at 10:06 AM, Antonio Borneo
<borneo.antonio@gmail.com> wrote:
> On Wed, Jul 4, 2012 at 3:13 AM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
>> This allows persisted tun device to be cleaned up for reuse.
>>
>> This is the minimal change to reach the goal using atexit(),
>> not sure it is the best way.
>
> Thanks for the patch set.
>
> I do not understand the reason for adding this patch.
> Current code in vpnc already runs close_tunnel() before exit, at least
> in all the cases I have tested. Your patch forces running
> close_tunnel() again for a second time.
> Cases of abrupt crash, e.g. segmentation fault, are not handled in
> current code neither in your patch, but such cases should be addressed
> by fixing directly the crash issue.
>
> I expect you have found some specific case in current code that fails
> to run close_tunnel().
> Or, the case of persistent tun device is not well cleared by current
> close_tunnel() and you need to run it twice. In this case the right
> target would be fixing existing code or script.
>
> Could you please report the specific use case that requires this patch?
>
> If you run vpnc with "--debug 2" you will get message
> S8 close_tunnel
> right before close_tunnel() is executed by existing code.
> This could help you to narrow the search.
>
> Best Regards,
> Antonio Borneo

Oh... I am so sorry!

The reason for this patch is if the vpnc exists because of error(),
for example, the wlan interface is restarted or any other error with
communications.

Calling error() at any point in code simply quits without proper cleanup.

The complex solution is to modify error() to properly terminate the
application including any cleanup code.

The simplest solution would be to use the atexit() to achieve more or
less the same.

It is true that in case of normal exit, the cleanup code will be
called too, I need to fix this one.

Alon.
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

This allows persisted tun device to be cleaned up for reuse.

This is the minimal change to reach the goal using atexit(),
not sure it is the best way.

Signed-off-by: Alon Bar-Lev <alon.barlev@gmail.com>
---
tunip.h | 1 +
vpnc.c | 24 +++++++++++++++++++++---
2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/tunip.h b/tunip.h
index 075a26f..78f1610 100644
--- a/tunip.h
+++ b/tunip.h
@@ -64,6 +64,7 @@ struct sa_block {
int tun_fd; /* fd to host via tun/tap */
char tun_name[IFNAMSIZ];
uint8_t tun_hwaddr[ETH_ALEN];
+ int tun_configured;

struct in_addr dst; /* ip of concentrator, must be set */
struct in_addr src; /* local ip, from getsockname() */
diff --git a/vpnc.c b/vpnc.c
index 206e6a9..0f9944e 100644
--- a/vpnc.c
+++ b/vpnc.c
@@ -373,18 +373,36 @@ static void setup_tunnel(struct sa_block *s)
}
}

+static struct sa_block *s_atexit_sa;
+static void close_tunnel(struct sa_block *s);
+static void atexit_close(void)
+{
+ if (s_atexit_sa != NULL) {
+ close_tunnel(s_atexit_sa);
+ s_atexit_sa = NULL;
+ }
+}
+
static void config_tunnel(struct sa_block *s)
{
setenv("VPNGATEWAY", inet_ntoa(s->dst), 1);
setenv("reason", "connect", 1);
system(config[CONFIG_SCRIPT]);
+ s->tun_configured = 1;
+ s_atexit_sa = s;
+ atexit(atexit_close);
}

static void close_tunnel(struct sa_block *s)
{
- setenv("reason", "disconnect", 1);
- system(config[CONFIG_SCRIPT]);
- tun_close(s->tun_fd, s->tun_name);
+ if (s->tun_configured) {
+ s->tun_configured = 0;
+ setenv("reason", "disconnect", 1);
+ system(config[CONFIG_SCRIPT]);
+ }
+ if (s->tun_fd != -1) {
+ tun_close(s->tun_fd, s->tun_name);
+ }
}

static int recv_ignore_dup(struct sa_block *s, void *recvbuf, size_t recvbufsize)
--
1.7.8.6

_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Wed, Jul 4, 2012 at 10:16 AM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
> On Wed, Jul 4, 2012 at 10:06 AM, Antonio Borneo
> <borneo.antonio@gmail.com> wrote:
>> On Wed, Jul 4, 2012 at 3:13 AM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
>>> This allows persisted tun device to be cleaned up for reuse.
>>>
>>> This is the minimal change to reach the goal using atexit(),
>>> not sure it is the best way.
>>
>> Thanks for the patch set.
>>
>> I do not understand the reason for adding this patch.
>> Current code in vpnc already runs close_tunnel() before exit, at least
>> in all the cases I have tested. Your patch forces running
>> close_tunnel() again for a second time.
>> Cases of abrupt crash, e.g. segmentation fault, are not handled in
>> current code neither in your patch, but such cases should be addressed
>> by fixing directly the crash issue.
>>
>> I expect you have found some specific case in current code that fails
>> to run close_tunnel().
>> Or, the case of persistent tun device is not well cleared by current
>> close_tunnel() and you need to run it twice. In this case the right
>> target would be fixing existing code or script.
>>
>> Could you please report the specific use case that requires this patch?
>>
>> If you run vpnc with "--debug 2" you will get message
>> S8 close_tunnel
>> right before close_tunnel() is executed by existing code.
>> This could help you to narrow the search.
>>
>> Best Regards,
>> Antonio Borneo
>
> Oh... I am so sorry!
>
> The reason for this patch is if the vpnc exists because of error(),
> for example, the wlan interface is restarted or any other error with
> communications.
>
> Calling error() at any point in code simply quits without proper cleanup.
>
> The complex solution is to modify error() to properly terminate the
> application including any cleanup code.
>
> The simplest solution would be to use the atexit() to achieve more or
> less the same.
>
> It is true that in case of normal exit, the cleanup code will be
> called too, I need to fix this one.
>
> Alon.

More specific example, when LAN interface is restarted:

send_delete_ipsec()->sendrecv_phase2()->sendrecv()->write()->error(1,...)

Regards,
Alon.
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Btw if we're really looking at the persistent tun handling, we should
try to sort out the mess that we have on FreeBSD. At the moment I don't
think vpnc even *works* without the net.link.tun.devfs_cloning
compatibility option being set, does it?

I've got some recent fixes for this in OpenConnect which I would have
pushed to vpnc, except it's still not quite right and the FreeBSD folks
have so far failed to actually come up with any suggestions for how (or
indeed *whether*) their stuff is *supposed* to work ☹

--
dwmw2

On Wed, Jul 4, 2012 at 11:48 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> Btw if we're really looking at the persistent tun handling, we should
> try to sort out the mess that we have on FreeBSD. At the moment I don't
> think vpnc even *works* without the net.link.tun.devfs_cloning
> compatibility option being set, does it?
>
> I've got some recent fixes for this in OpenConnect which I would have
> pushed to vpnc, except it's still not quite right and the FreeBSD folks
> have so far failed to actually come up with any suggestions for how (or
> indeed *whether*) their stuff is *supposed* to work ☹

Oh... I don't use *BSD, so I cannot really help in this regard.
But as far as I seen, *BSD can be treated as always persisted tun...
the vpnc-script can be adjusted not to create and not to destroy.

Alon.
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Wed, 2012-07-04 at 11:53 +0300, Alon Bar-Lev wrote:
> Oh... I don't use *BSD, so I cannot really help in this regard.

Neither do I, but it's simple enough to install it in a VM.

> But as far as I seen, *BSD can be treated as always persisted tun...
> the vpnc-script can be adjusted not to create and not to destroy.

We probably want a way for vpnc/openconnect to *tell* the vpnc-script
whether to destroy the device, depending on whether it created it for
itself or not. Either a separate call with 'reason=destroy' or another
environment variable which changes the behaviour of the existing
disconnect call.

I think I prefer 'reason=destroy', because it can also happen after the
tun fd is closed, thus avoiding a deadlock that happens on some FreeBSD
kernels. And compatibility with older vpnc-script is OK because those
will destroy the device in 'disconnect' unconditionally, then just do
nothing when invoked against for 'destroy'.

--
dwmw2

On Wed, Jul 4, 2012 at 12:26 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Wed, 2012-07-04 at 11:53 +0300, Alon Bar-Lev wrote:
>> Oh... I don't use *BSD, so I cannot really help in this regard.
>
> Neither do I, but it's simple enough to install it in a VM.
>
>> But as far as I seen, *BSD can be treated as always persisted tun...
>> the vpnc-script can be adjusted not to create and not to destroy.
>
> We probably want a way for vpnc/openconnect to *tell* the vpnc-script
> whether to destroy the device, depending on whether it created it for
> itself or not. Either a separate call with 'reason=destroy' or another
> environment variable which changes the behaviour of the existing
> disconnect call.
>
> I think I prefer 'reason=destroy', because it can also happen after the
> tun fd is closed, thus avoiding a deadlock that happens on some FreeBSD
> kernels. And compatibility with older vpnc-script is OK because those
> will destroy the device in 'disconnect' unconditionally, then just do
> nothing when invoked against for 'destroy'.

OK, but how do you suggest telling?
A new option to the vpnc?
Or just specify a parameter to the script?
---
Script xxxxx --persist-tunnel
---

Alon.
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Wed, 2012-07-04 at 13:39 +0300, Alon Bar-Lev wrote:
> > We probably want a way for vpnc/openconnect to *tell* the vpnc-script
> > whether to destroy the device, depending on whether it created it for
> > itself or not…

> OK, but how do you suggest telling?
> A new option to the vpnc?

There's already an option to specify an interface to use. If vpnc is
given that option, and it doesn't have to create the device for itself,
then it should *not* tell the script to destroy the device. In all other
cases, it *should*.

> Or just specify a parameter to the script?

As I said, I think I prefer an extra invocation with 'reason=destroy'.

--
dwmw2

On Wed, Jul 4, 2012 at 1:46 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Wed, 2012-07-04 at 13:39 +0300, Alon Bar-Lev wrote:
>> > We probably want a way for vpnc/openconnect to *tell* the vpnc-script
>> > whether to destroy the device, depending on whether it created it for
>> > itself or not…
>
>> OK, but how do you suggest telling?
>> A new option to the vpnc?
>
> There's already an option to specify an interface to use. If vpnc is
> given that option, and it doesn't have to create the device for itself,
> then it should *not* tell the script to destroy the device. In all other
> cases, it *should*.

OK.
Now I understand.

>
>> Or just specify a parameter to the script?
>
> As I said, I think I prefer an extra invocation with 'reason=destroy'.

Yes, I referred to the way the script should be told, but you already
replied that above...

I miss the part where at *BSD the tun is created.

I will prepare a patch later this day.

Alon
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Wed, Jul 4, 2012 at 3:55 PM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
>> The reason for this patch is if the vpnc exists because of error(),
>> for example, the wlan interface is restarted or any other error with
>> communications.
>>
>> Calling error() at any point in code simply quits without proper cleanup.
>>
>> The complex solution is to modify error() to properly terminate the
>> application including any cleanup code.
>>
>> The simplest solution would be to use the atexit() to achieve more or
>> less the same.
> ...
> More specific example, when LAN interface is restarted:
>
> send_delete_ipsec()->sendrecv_phase2()->sendrecv()->write()->error(1,...)

I see! Did not considered calling error().

A cleaner solution would be to replace every error() call with a
wrapper, e.g. vpnc_error(), that will perform the clean-up before
quit.

Best Regards,
Antonio Borneo
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Wed, Jul 4, 2012 at 2:49 PM, Antonio Borneo <borneo.antonio@gmail.com> wrote:
> On Wed, Jul 4, 2012 at 3:55 PM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
>>> The reason for this patch is if the vpnc exists because of error(),
>>> for example, the wlan interface is restarted or any other error with
>>> communications.
>>>
>>> Calling error() at any point in code simply quits without proper cleanup.
>>>
>>> The complex solution is to modify error() to properly terminate the
>>> application including any cleanup code.
>>>
>>> The simplest solution would be to use the atexit() to achieve more or
>>> less the same.
>> ...
>> More specific example, when LAN interface is restarted:
>>
>> send_delete_ipsec()->sendrecv_phase2()->sendrecv()->write()->error(1,...)
>
> I see! Did not considered calling error().
>
> A cleaner solution would be to replace every error() call with a
> wrapper, e.g. vpnc_error(), that will perform the clean-up before
> quit.

I think it is similar to the atexit solution as it will force the use
of global variables anyway....

A cleaner solution would be not to quit at the middle of a program but
return an error to caller.

Alon.
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Wed, Jul 4, 2012 at 2:54 PM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
> On Wed, Jul 4, 2012 at 2:49 PM, Antonio Borneo <borneo.antonio@gmail.com> wrote:
>> On Wed, Jul 4, 2012 at 3:55 PM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
>>>> The reason for this patch is if the vpnc exists because of error(),
>>>> for example, the wlan interface is restarted or any other error with
>>>> communications.
>>>>
>>>> Calling error() at any point in code simply quits without proper cleanup.
>>>>
>>>> The complex solution is to modify error() to properly terminate the
>>>> application including any cleanup code.
>>>>
>>>> The simplest solution would be to use the atexit() to achieve more or
>>>> less the same.
>>> ...
>>> More specific example, when LAN interface is restarted:
>>>
>>> send_delete_ipsec()->sendrecv_phase2()->sendrecv()->write()->error(1,...)
>>
>> I see! Did not considered calling error().
>>
>> A cleaner solution would be to replace every error() call with a
>> wrapper, e.g. vpnc_error(), that will perform the clean-up before
>> quit.
>
> I think it is similar to the atexit solution as it will force the use
> of global variables anyway....
>
> A cleaner solution would be not to quit at the middle of a program but
> return an error to caller.
>
> Alon.

I modified the patch, still uses atexit as I am not convinced it is
different than doing the same at error().

Patch series is at [1].
Path is at [2].

If you like me to re-send raw patch into list just tell.

Alon.

[1] https://github.com/alonbl/vpnc/compare/master...unprivileged
[2] https://github.com/alonbl/vpnc/commit/8e196e0512cae8deb53393d61e990af8999c1d3a
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Sat, 2012-07-07 at 18:37 +0300, Alon Bar-Lev wrote:
> Patch series is at [1].
> [1] https://github.com/alonbl/vpnc/compare/master...unprivileged

Please could I have the vpnc-script changes as a pull request against
the vpnc-scripts.git repository at
http://git.infradead.org/users/dwmw2/vpnc-scripts.git

Thanks.

--
dwmw2

On Sat, Jul 7, 2012 at 7:12 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Sat, 2012-07-07 at 18:37 +0300, Alon Bar-Lev wrote:
>> Patch series is at [1].
>> [1] https://github.com/alonbl/vpnc/compare/master...unprivileged
>
> Please could I have the vpnc-script changes as a pull request against
> the vpnc-scripts.git repository at
> http://git.infradead.org/users/dwmw2/vpnc-scripts.git
>
> Thanks.

No problems[1].
Use the unprivileged branch.
I didn't quite understand what reconnect is, not sure if I need to
delete the ip address in disconnect or in reconnect, I left this in
disconnect.

Alon.

[1] https://github.com/alonbl/vpnc-scripts/compare/master...unprivileged
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

ping...

On Sat, Jul 7, 2012 at 7:35 PM, Alon Bar-Lev <alon.barlev@gmail.com> wrote:
> On Sat, Jul 7, 2012 at 7:12 PM, David Woodhouse <dwmw2@infradead.org> wrote:
>> On Sat, 2012-07-07 at 18:37 +0300, Alon Bar-Lev wrote:
>>> Patch series is at [1].
>>> [1] https://github.com/alonbl/vpnc/compare/master...unprivileged
>>
>> Please could I have the vpnc-script changes as a pull request against
>> the vpnc-scripts.git repository at
>> http://git.infradead.org/users/dwmw2/vpnc-scripts.git
>>
>> Thanks.
>
> No problems[1].
> Use the unprivileged branch.
> I didn't quite understand what reconnect is, not sure if I need to
> delete the ip address in disconnect or in reconnect, I left this in
> disconnect.
>
> Alon.
>
> [1] https://github.com/alonbl/vpnc-scripts/compare/master...unprivileged
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/