Hi Herbert, I have a couple of comments, please see below.
> The reason for this is that the problem that Hugo uncovered is only
> the tip of the iceberg. The real problem is that when we removed the
> dependency of ipip on xfrm4_tunnel we didn't really consider the module
> case at all.
>
> For instance, as it is it's possible to build both ipip and xfrm4_tunnel
> as modules and if the latter is loaded then ipip simply won't load.
Yes, this was another problem i pointed out. But the real issue i
wanted to fix was the soft-lockup with module ip6_tunnel being used
when having xfrm6_tunnel compiled as module.
> After considering the alternatives I've decided that the best way out of
> this is to restore the dependency of ipip on the non-xfrm-specific part
> of xfrm4_tunnel. This is acceptable IMHO because the intention of the
> removal was really to be able to use ipip without the xfrm subsystem.
> This is still preserved by this patch.
This looks OK to me, but i think that maybe not calling the methods
xfrm{4,6}_tunnel_register and deregister would be better as tunnel{4,6}
is not really related to xfrm. Maybe something like tunnel6_register
would be better and more consistent with the file naming.
One small issue with the patch is tunnel6_rcv's return of 0 when no
handler handles the packet which will increase IPSTATS_MIB_INDELIVERS.
I guess -1 should be returned on error. I was checking ip_input and the
expected behaviour there is different, any value != 0 will trigger a
re-submit.
Also, there's a bigger problem which is the ICMP error generation. I
acknowledge you mentioned that this is work for another patch but i
would like to comment it now. Right now you kept the icmpv6_send in
ip6_tunnel's discard path, which doesn't work that well if you have
another tunnel handler following like xfrm6_tunnel. I would suggest
having the icmpv6_send in tunnel6_rcv's error path, so if no handler
was able to process the packet, the source receives the DEST_UNREACH
error. I'm not sure if this is desirable in IPSec but i'm sure you'll
be able to comment on that.
Thanks,
Hugo
signature.asc
Description: Digital signature
