Re: svn commit: r256519 - in head/sys: net netatalk netinet netinet6 netipx

> On Sep 30, 2025, at 10:59 AM, Mateusz Guzik <mjguzik@gmail.com> wrote:
> 
> On Tue, Sep 30, 2025 at 4:38 AM Zhenlei Huang <zlei@freebsd.org <mailto:zlei@freebsd.org>> wrote:
>> 
>> 
>> 
>>> On Sep 29, 2025, at 10:29 PM, Mateusz Guzik <mjguzik@gmail.com> wrote:
>>> 
>>> On Tue, Oct 15, 2013 at 12:31 PM Gleb Smirnoff <glebius@freebsd.org> wrote:
>>>> 
>>>> Author: glebius
>>>> Date: Tue Oct 15 10:31:42 2013
>>>> New Revision: 256519
>>>> URL: http://svnweb.freebsd.org/changeset/base/256519
>>>> 
>>>> Log:
>>>>   Remove ifa_init() and provide ifa_alloc() that will allocate and setup
>>>> struct ifaddr internally.
>>>> 
>>>> Sponsored by: Netflix
>>>> Sponsored by: Nginx, Inc.
>>>> 
>>>> 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
>>>> 
>>>> Modified: head/sys/net/if.c
>>>> ==============================================================================
>>>> --- 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 = sizeof(*sdl);
>>>>               socksize = roundup2(socksize, sizeof(long));
>>>>               ifasize = sizeof(*ifa) + 2 * socksize;
>>>> -               ifa = malloc(ifasize, M_IFADDR, M_WAITOK | M_ZERO);
>>>> -               ifa_init(ifa);
>>>> +               ifa = ifa_alloc(ifasize, M_WAITOK);
>>>>               sdl = (struct sockaddr_dl *)(ifa + 1);
>>>>               sdl->sdl_len = socksize;
>>>>               sdl->sdl_family = 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 >= sizeof(struct ifaddr),
>>>> +           ("%s: invalid size %zu", __func__, size));
>>>> +
>>> 
>>> We have crashes stemming from this:
>>> 
>>> panic: ifa_alloc: invalid size 16
>>> 
>>> 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
>>> 
>>> in6_ifadd has:
>>>                       struct in6_addr taddr;
>>>                       ifa = ifa_alloc(sizeof(taddr), M_WAITOK);
>>> 
>>> should the assert be simply removed?
>> 
>> Hi Mateusz,
>> 
>> I believe you just found a bug.
>> 
>> Try the following patch please,
>> 
>> --- a/sys/netinet6/nd6_rtr.c
>> +++ b/sys/netinet6/nd6_rtr.c
>> @@ -1243,8 +1243,7 @@ in6_ifadd(struct nd_prefixctl *pr, int mcast)
>> 
>>                /* No suitable LL address, get the ifid directly */
>>                if (ifid_addr == NULL) {
>> -                       struct in6_addr taddr;
>> -                       ifa = ifa_alloc(sizeof(taddr), M_WAITOK);
>> +                       ifa = ifa_alloc(sizeof(struct in6_ifaddr), M_WAITOK);
>>                        if (ifa) {
>>                                ib = (struct in6_ifaddr *)ifa;
>>                                ifid_addr = &ib->ia_addr.sin6_addr;
>> 
> 
> Thanks for the patch. I don't have means to readily test it.
> 
> This panic was getting in the way of looking at another panic so I did
> not pay much attention.
> 
> But now that you point this out, I don't think the patch is sufficient.
> 
> in6_get_ifid starts with NET_EPOCH_ASSERT.
> 
> At the same time malloc(..., M_WAITOK) is illegal to call from an epoch section.

So M_NOWAIT should be used, instead of M_WAITOK .

> 
> I don't know if in6_ifadd is called within net epoch, either way the
> above two are clearly contradictory.
> 
> Is there are a reason to malloc this in the first place?

I have not look into the code throughly. That was introduced via commit https://cgit.freebsd.org/src/commit/?id=9e792f7ef7298080c058fbc2d36a4e60e596dae9 .

See https://reviews.freebsd.org/D51778 for more context.

Best regards,
Zhenlei