Re: [PATCHv2 net] ipv6: sr: fix invalid unregister error path

[Simon Horman <horms@kernel.org>]

On Wed, May 08, 2024 at 10:55:02AM +0800, Hangbin Liu wrote:
> The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL
> is not defined. In that case if seg6_hmac_init() fails, the
> genl_unregister_family() isn't called.
> 
> At the same time, add seg6_local_exit() and fix the genl unregister order
> in seg6_exit().

It seems that this fixes two, or perhaps three different problems.
Perhaps we should consider two or three patches?

Also, could you explain the implications of changing the unregister order
in the patch description: it should describe why a change is made.

> Fixes: 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")

I agree that the current manifestation of the first problem
was introduced. But didn't a very similar problem exist before then?
I suspect the fixes tag should refer to an earlier commit.

> Reported-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

I think these bugs are pretty good examples of why not
to sprinkle #ifdef inside of functions - it makes the
logic hard to reason with.

So while I agree that a minimal fix, along the lines of this patch, is
suitable for 'net'. Could we consider, as a follow-up, refactoring the code
to remove this #ifdef spaghetti? F.e. by providing dummy implementations
of seg6_iptunnel_init()/seg6_iptunnel_exit() and so on.

-- 
pw-bot: changes-requested