Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

session.pre-down is run after the interface is removed #56

Open
RalfJung opened this issue Oct 30, 2017 · 6 comments
Open

session.pre-down is run after the interface is removed #56

RalfJung opened this issue Oct 30, 2017 · 6 comments

Comments

@RalfJung
Copy link
Member

We have the following line in our pre-down hook:

/sbin/brctl delif saarVPN $INTERFACE

With the old version of the broker (before the rewrite, see e.g. https://github.com/ffrl/tunneldigger/) on old kernels (3.16.0), that worked just fine. However, on kernel 4.9.30 and with the latest broker, we now have errors in the log:

(session.pre-down/26205) interface l2tp2221 does not exist!

I am not seeing these errors with the new broker on an old kernel. So it seems the kernel update is the reason here, not the broker update.

That is strange, isn't it? How would the kernel even know already that the tunnel is dead? Does L2TP have in-band signaling for tearing down the tunnel?

@RalfJung
Copy link
Member Author

Or maybe this is a race condition? If I read that code in hooks.py correctly, it is asynchronous. So the broker actually goes on and deletes the interface in Tunnel.close while the hook is still running.

@kostko
Copy link
Member

kostko commented Oct 30, 2017

Or maybe this is a race condition? If I read that code in hooks.py correctly, it is asynchronous. So the broker actually goes on and deletes the interface in Tunnel.close while the hook is still running.

This is true and in this case we would need to wait for the hook to finish executing before proceeding with tunnel teardown.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 30, 2017

Right. I think, actually, that for our purpose a post-down hook is good enough, but I would have to check. I mean, deleting the interface will remove it from the bridge, so we don't have to do that in the hook.

@kostko
Copy link
Member

kostko commented Oct 30, 2017

Yes, for us such behavior was ok as well and this is the reason why I didn't implement "blocking" hooks. Also, a hook could in theory loop forever and block the tunnel from being freed.

@RalfJung
Copy link
Member Author

Well, but then it shouldn't be called pre-down hook. Removing that hook would be the more honest thing to do.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 30, 2017

Also, a hook could in theory loop forever and block the tunnel from being freed.

Well, sure. An admin can always misconfigure their server.
The old broker that we still use on our other two servers uses blocking hooks, and that's working just fine. It's not like hooks usually do stuff on the network.

Actually, we have hooks that rely on this blocking behavior. They modify iptables and could mess things up when running concurrently. (Yes, it's a bad hack, but it's needed to work around a bad firewall.) So this [undocumented] change from synchronous to asynchronous hooks could cause trouble for us.

And even for the non-hacky part... if the tunnel is only added to the bridge asynchronously, doesn't that mean that the client could already be sending data into the tunnel before it is even plugged into the bridge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants