Date: Sun, 5 Oct 2025 11:53:16 +0200 From: Kristof Provost <kp@freebsd.org> To: "Bjoern A. Zeeb" <bz@freebsd.org> Cc: Zhenlei Huang <zlei@freebsd.org>, Mateusz Guzik <mjguzik@gmail.com>, Gleb Smirnoff <glebius@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: svn commit: r256519 - in head/sys: net netatalk netinet netinet6 netipx Message-ID: <A43F4D0B-F517-4F60-B929-C04EE546E84E@freebsd.org> In-Reply-To: <qq186r8p-4q40-7o1-4oss-1rnprn2r3p5@SerrOFQ.bet> References: <qq186r8p-4q40-7o1-4oss-1rnprn2r3p5@SerrOFQ.bet>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 4 Oct 2025, at 18:13, Bjoern A. Zeeb <bz@freebsd.org> wrote: >=20 > =EF=BB=BFOn Tue, 30 Sep 2025, Zhenlei Huang wrote: >=20 >>>> On Sep 30, 2025, at 10:59 AM, Mateusz Guzik <mjguzik@gmail.com> wrote: >>>=20 >>> On Tue, Sep 30, 2025 at 4:38=E2=80=AFAM Zhenlei Huang <zlei@freebsd.org <= mailto:zlei@freebsd.org>> wrote: >>>>=20 >>>>=20 >>>>=20 >>>>> On Sep 29, 2025, at 10:29 PM, Mateusz Guzik <mjguzik@gmail.com> wrote:= >>>>>=20 >>>>> On Tue, Oct 15, 2013 at 12:31=E2=80=AFPM Gleb Smirnoff <glebius@freebs= d.org> wrote: >>>>>>=20 >>>>>> Author: glebius >>>>>> Date: Tue Oct 15 10:31:42 2013 >>>>>> New Revision: 256519 >>>>>> URL: http://svnweb.freebsd.org/changeset/base/256519 >>>>>>=20 >>>>>> Log: >>>>>> Remove ifa_init() and provide ifa_alloc() that will allocate and set= up >>>>>> struct ifaddr internally. >>>>>>=20 >>>>>> Sponsored by: Netflix >>>>>> Sponsored by: Nginx, Inc. >>>>>>=20 >>>>>> Modified: >>>>>> head/sys/net/if.c >>>>>> head/sys/net/if_var.h >>>>>> head/sys/netatalk/at_control.c >>>>>> head/sys/netinet/in.c >>>>>> head/sys/netinet6/in6.c >>>>>> head/sys/netipx/ipx.c >>>>>>=20 >>>>>> Modified: head/sys/net/if.c >>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>>>> --- head/sys/net/if.c Tue Oct 15 10:19:24 2013 (r256518) >>>>>> +++ head/sys/net/if.c Tue Oct 15 10:31:42 2013 (r256519) >>>>>> @@ -633,8 +633,7 @@ if_attach_internal(struct ifnet *ifp, in >>>>>> socksize =3D sizeof(*sdl); >>>>>> socksize =3D roundup2(socksize, sizeof(long)); >>>>>> ifasize =3D sizeof(*ifa) + 2 * socksize; >>>>>> - ifa =3D malloc(ifasize, M_IFADDR, M_WAITOK | M_ZERO);= >>>>>> - ifa_init(ifa); >>>>>> + ifa =3D ifa_alloc(ifasize, M_WAITOK); >>>>>> sdl =3D (struct sockaddr_dl *)(ifa + 1); >>>>>> sdl->sdl_len =3D socksize; >>>>>> sdl->sdl_family =3D AF_LINK; >>>>>> @@ -1417,13 +1416,23 @@ if_maddr_runlock(struct ifnet *ifp) >>>>>> /* >>>>>> * Initialization, destruction and refcounting functions for ifaddrs. >>>>>> */ >>>>>> -void >>>>>> -ifa_init(struct ifaddr *ifa) >>>>>> +struct ifaddr * >>>>>> +ifa_alloc(size_t size, int flags) >>>>>> { >>>>>> + struct ifaddr *ifa; >>>>>> + >>>>>> + KASSERT(size >=3D sizeof(struct ifaddr), >>>>>> + ("%s: invalid size %zu", __func__, size)); >>>>>> + >>>>>=20 >>>>> We have crashes stemming from this: >>>>>=20 >>>>> panic: ifa_alloc: invalid size 16 >>>>>=20 >>>>> panic() at panic+0x43/frame 0xfffffe009e777760 >>>>> ifa_alloc() at ifa_alloc+0xd6/frame 0xfffffe009e777780 >>>>> in6_ifadd() at in6_ifadd+0xd8/frame 0xfffffe009e7778a0 >>>>> nd6_ra_input() at nd6_ra_input+0x1023/frame 0xfffffe009e777a80 >>>>> icmp6_input() at icmp6_input+0x5b6/frame 0xfffffe009e777c00 >>>>> ip6_input() at ip6_input+0xc94/frame 0xfffffe009e777ce0 >>>>> sppp_input() at sppp_input+0x502/frame 0xfffffe009e777d70 >>>>> pppoe_data_input() at pppoe_data_input+0x1e7/frame 0xfffffe009e777de0 >>>>> swi_net() at swi_net+0x19b/frame 0xfffffe009e777e60 >>>>> ithread_loop() at ithread_loop+0x266/frame 0xfffffe009e777ef0 >>>>> fork_exit() at fork_exit+0x82/frame 0xfffffe009e777f30 >>>>> fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe009e777f30 >>>>>=20 >>>>> in6_ifadd has: >>>>> struct in6_addr taddr; >>>>> ifa =3D ifa_alloc(sizeof(taddr), M_WAITOK); >>>>>=20 >>>>> should the assert be simply removed? >>>>=20 >>>> Hi Mateusz, >>>>=20 >>>> I believe you just found a bug. >>>>=20 >>>> Try the following patch please, >>>>=20 >>>> --- a/sys/netinet6/nd6_rtr.c >>>> +++ b/sys/netinet6/nd6_rtr.c >>>> @@ -1243,8 +1243,7 @@ in6_ifadd(struct nd_prefixctl *pr, int mcast) >>>>=20 >>>> /* No suitable LL address, get the ifid directly */ >>>> if (ifid_addr =3D=3D NULL) { >>>> - struct in6_addr taddr; >>>> - ifa =3D ifa_alloc(sizeof(taddr), M_WAITOK); >>>> + ifa =3D ifa_alloc(sizeof(struct in6_ifaddr), M_= WAITOK); >>>> if (ifa) { >>>> ib =3D (struct in6_ifaddr *)ifa; >>>> ifid_addr =3D &ib->ia_addr.sin6_addr; >>>>=20 >>>=20 >>> Thanks for the patch. I don't have means to readily test it. >>>=20 >>> This panic was getting in the way of looking at another panic so I did >>> not pay much attention. >>>=20 >>> But now that you point this out, I don't think the patch is sufficient. >>>=20 >>> in6_get_ifid starts with NET_EPOCH_ASSERT. >>>=20 >>> At the same time malloc(..., M_WAITOK) is illegal to call from an epoch s= ection. >>=20 >> So M_NOWAIT should be used, instead of M_WAITOK . >>=20 >>>=20 >>> I don't know if in6_ifadd is called within net epoch, either way the >>> above two are clearly contradictory. >>>=20 >>> Is there are a reason to malloc this in the first place? >>=20 >> I have not look into the code throughly. That was introduced via commit h= ttps://cgit.freebsd.org/src/commit/?id=3D9e792f7ef7298080c058fbc2d36a4e60e59= 6dae9 . >>=20 >> See https://reviews.freebsd.org/D51778 for more context. >=20 > Wait, so first the bug came in on a controversial chnage to which multiple= > FreeBSD people said they were not sure what it is good for and then we > get panic reports from the same source about their own code? >=20 > I think it is time to backout the original at this point.=20 This panic is already fixed, and I added a test case to exercise (and illust= rate) the relevant code path.=20 At this point I see no reason to revert, especially as the revert is non-tri= vial. As discussed with secteam as well.=20 =E2=80=94=20 Kristof=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?A43F4D0B-F517-4F60-B929-C04EE546E84E>