Re: svn commit: r256519 - in head/sys: net netatalk netinet netinet6 netipx
Date: Sat, 04 Oct 2025 16:13:32 UTC
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. At least then
the commit history can be fixed?
/bz
--
Bjoern A. Zeeb r15:7