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

> On 4 Oct 2025, at 18:13, Bjoern A. Zeeb <bz@freebsd.org> wrote:
> 
> On Tue, 30 Sep 2025, Zhenlei Huang wrote:
> 
>>>> 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.
> 
> 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?
> 
> I think it is time to backout the original at this point. 

This panic is already fixed, and I added a test case to exercise (and illustrate) the relevant code path. 

At this point I see no reason to revert, especially as the revert is non-trivial. As discussed with secteam as well. 

— 
Kristof