netdev - Re: [Patch net-next] ipv4: initialize ra_mutex in inet_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpVz_bxDLPZder7L+Tm4Qqzb3PTncf7BbyQp=7v9WYuB3Q@mail.gmail.com>
Date:   Wed, 19 Sep 2018 14:28:55 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [Patch net-next] ipv4: initialize ra_mutex in inet_init_net()

On Wed, Sep 19, 2018 at 1:25 AM Kirill Tkhai <ktkhai@...tuozzo.com> wrote:
>
> On 18.09.2018 23:17, Cong Wang wrote:
> > On Mon, Sep 17, 2018 at 12:25 AM Kirill Tkhai <ktkhai@...tuozzo.com> wrote:
> >> In inet_init() the order of registration is:
> >>
> >>         ip_mr_init();
> >>         init_inet_pernet_ops();
> >>
> >> This means, ipmr_net_ops pernet operations are before af_inet_ops
> >> in pernet_list. So, there is a theoretical probability, sometimes
> >> in the future, we will have a problem during a fail of net initialization.
> >>
> >> Say,
> >>
> >> setup_net():
> >>         ipmr_net_ops->init() returns 0
> >>         xxx->init()          returns error
> >> and then we do:
> >>         ipmr_net_ops->exit(),
> >>
> >> which could touch ra_mutex (theoretically).
> >
> > How could ra_mutex be touched in this scenario?
> >
> > ra_mutex is only used in ip_ra_control() which is called
> > only by {get,set}sockopt(). I don't see anything related
> > to netns exit() path here.
>
> Currently, it is not touched. But it's an ordinary practice,
> someone closes sockets in pernet ->exit methods. For example,
> we close percpu icmp sockets in icmp_sk_exit(), which are
> also of RAW type, and there is also called ip_ra_control()
> for them. Yes, they differ by their protocol; icmp sockets
> are of IPPROTO_ICMP protocol, while ip_ra_control() acts
> on IPPROTO_RAW sockets, but it's not good anyway. This does
> not look reliable for the future. In case of someone changes
> something here, we may do not notice this for the long time,
> while some users will meet bugs on their places.

First of all, we only consider current code base. Even if you
really planned to changed this in the future, it would be still your
responsibility to take care of it. Why? Simple, FIFO. My patch
comes ahead of any future changes here, obviously.

Secondly, it is certainly not hard to notice. I am pretty sure
you would get a warning/crash if this bug triggered.



>
> Problems on error paths is not easy to detect on testing,
> while user may meet them. We had issue of same type with
> uninitialized xfrm_policy_lock. It was introduced in 2013,
> while the problem was found only in 2017:

Been there, done that, I've fixed multiple IPv6 init code
error path. This is not where we disagree, by the way.


>
>         introduced by 283bc9f35bbb
>         fixed      by c282222a45cb
>
> (Last week I met it on RH7 kernel, which still has no a fix.
>  But this talk is not about distribution kernels, just about
>  the way).
>
> I just want to say if someone makes some creativity on top
> of this code, it will be to more friendly from us to him/her
> to not force this person to think about such not obvious details,
> but just to implement nice architecture right now.

You keep saying nice architecture, how did you architect
ipv4.ra_mutex into net/core/net_namespace.c? It is apparently
not nice.

Good luck with your future.

I am tired, let's just drop it. I have no interest to fix your mess.

Thanks!