NET_LOCK and trunk detach

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

NET_LOCK and trunk detach

Sven F.
Hello,

I am running some trunk interfaces in a multi core environment,
it's a slightly modified version, i have a few NET_ASSERT_LOCKED();
suspecting some multi core shenanigans, which i guess was confirmed:
(unsure the have X meaning, but i ' m pretty sure 256 is very wrong)
the if_trunk.c locking is completely unmodified
The code is 6.7-stable

splassert: lacp_detach: want 2 have 0
splassert: lacp_detach: want 2 have 0
splassert: lacp_detach: want 2 have 256

I noticed : trunk_clone_destroy ,call

    if (tr->tr_proto != TRUNK_PROTO_NONE)
        tr->tr_detach(tr);

outside the lock, and that trunk_ioctl call it

        if (tr->tr_proto != TRUNK_PROTO_NONE)
            error = tr->tr_detach(tr);

but ioctl is as far as i understand locked.
I'm unsure if the difficult and amazing unlocking work
did an oopsies or if ioctl is already assumed unlocked.

Kindly inform me.
Best regards, thank you for reading.

Reply | Threaded
Open this post in threaded view
|

Re: NET_LOCK and trunk detach

Vitaliy Makkoveev-2
On Tue, Jul 28, 2020 at 04:10:01PM -0400, sven falempin wrote:

> Hello,
>
> I am running some trunk interfaces in a multi core environment,
> it's a slightly modified version, i have a few NET_ASSERT_LOCKED();
> suspecting some multi core shenanigans, which i guess was confirmed:
> (unsure the have X meaning, but i ' m pretty sure 256 is very wrong)
> the if_trunk.c locking is completely unmodified
> The code is 6.7-stable
>
> splassert: lacp_detach: want 2 have 0
> splassert: lacp_detach: want 2 have 0
> splassert: lacp_detach: want 2 have 256
>
> I noticed : trunk_clone_destroy ,call
>
>     if (tr->tr_proto != TRUNK_PROTO_NONE)
>         tr->tr_detach(tr);
>
> outside the lock, and that trunk_ioctl call it
>
>         if (tr->tr_proto != TRUNK_PROTO_NONE)
>             error = tr->tr_detach(tr);
>
> but ioctl is as far as i understand locked.
> I'm unsure if the difficult and amazing unlocking work
> did an oopsies or if ioctl is already assumed unlocked.
>
> Kindly inform me.
> Best regards, thank you for reading.
>

lacp_detach() touches nothing which requires NET_LOCK(). What is the
reason you placed assertion to lacp_detach()?

Reply | Threaded
Open this post in threaded view
|

Re: NET_LOCK and trunk detach

Sven F.
On Tue, Jul 28, 2020 at 4:42 PM Vitaliy Makkoveev <[hidden email]> wrote:

>
> On Tue, Jul 28, 2020 at 04:10:01PM -0400, sven falempin wrote:
> > Hello,
> >
> > I am running some trunk interfaces in a multi core environment,
> > it's a slightly modified version, i have a few NET_ASSERT_LOCKED();
> > suspecting some multi core shenanigans, which i guess was confirmed:
> > (unsure the have X meaning, but i ' m pretty sure 256 is very wrong)
> > the if_trunk.c locking is completely unmodified
> > The code is 6.7-stable
> >
> > splassert: lacp_detach: want 2 have 0
> > splassert: lacp_detach: want 2 have 0
> > splassert: lacp_detach: want 2 have 256
> >
> > I noticed : trunk_clone_destroy ,call
> >
> >     if (tr->tr_proto != TRUNK_PROTO_NONE)
> >         tr->tr_detach(tr);
> >
> > outside the lock, and that trunk_ioctl call it
> >
> >         if (tr->tr_proto != TRUNK_PROTO_NONE)
> >             error = tr->tr_detach(tr);
> >
> > but ioctl is as far as i understand locked.
> > I'm unsure if the difficult and amazing unlocking work
> > did an oopsies or if ioctl is already assumed unlocked.
> >
> > Kindly inform me.
> > Best regards, thank you for reading.
> >
>
> lacp_detach() touches nothing which requires NET_LOCK(). What is the
> reason you placed assertion to lacp_detach()?

<<it's a slightly modified version, i have a few NET_ASSERT_LOCKED()>>

the lacp_detach is not yours, i put them here because i have a NULL pointer
popping in other 'driver' callback.

I'm tracking this and trying to understand  how this memory is 'nullified'
mid function.

I do not think putting NET_ASSERT_LOCKED can be harmful in any way.
If so please tell me.

I am just tracking  a bug  and noticed these detach locking strangeness.

Reply | Threaded
Open this post in threaded view
|

Re: NET_LOCK and trunk detach

Vitaliy Makkoveev-3


> On 29 Jul 2020, at 00:09, sven falempin <[hidden email]> wrote:
>
> On Tue, Jul 28, 2020 at 4:42 PM Vitaliy Makkoveev <[hidden email]> wrote:
>>
>> On Tue, Jul 28, 2020 at 04:10:01PM -0400, sven falempin wrote:
>>> Hello,
>>>
>>> I am running some trunk interfaces in a multi core environment,
>>> it's a slightly modified version, i have a few NET_ASSERT_LOCKED();
>>> suspecting some multi core shenanigans, which i guess was confirmed:
>>> (unsure the have X meaning, but i ' m pretty sure 256 is very wrong)
>>> the if_trunk.c locking is completely unmodified
>>> The code is 6.7-stable
>>>
>>> splassert: lacp_detach: want 2 have 0
>>> splassert: lacp_detach: want 2 have 0
>>> splassert: lacp_detach: want 2 have 256
>>>
>>> I noticed : trunk_clone_destroy ,call
>>>
>>>    if (tr->tr_proto != TRUNK_PROTO_NONE)
>>>        tr->tr_detach(tr);
>>>
>>> outside the lock, and that trunk_ioctl call it
>>>
>>>        if (tr->tr_proto != TRUNK_PROTO_NONE)
>>>            error = tr->tr_detach(tr);
>>>
>>> but ioctl is as far as i understand locked.
>>> I'm unsure if the difficult and amazing unlocking work
>>> did an oopsies or if ioctl is already assumed unlocked.
>>>
>>> Kindly inform me.
>>> Best regards, thank you for reading.
>>>
>>
>> lacp_detach() touches nothing which requires NET_LOCK(). What is the
>> reason you placed assertion to lacp_detach()?
>
> <<it's a slightly modified version, i have a few NET_ASSERT_LOCKED()>>
>
> the lacp_detach is not yours, i put them here because i have a NULL pointer
> popping in other 'driver' callback.
>
> I'm tracking this and trying to understand  how this memory is 'nullified'
> mid function.

I have no telepathy skills. Sorry. If you have panics send please dmesg(8)
output and instruction for reproduce.

>
> I do not think putting NET_ASSERT_LOCKED can be harmful in any way.
> If so please tell me.
>

What is the data you think needs be protected by netlock?

> I am just tracking  a bug  and noticed these detach locking strangeness.
>

Reply | Threaded
Open this post in threaded view
|

Re: NET_LOCK and trunk detach

Sven F.
On Tue, Jul 28, 2020 at 5:27 PM Vitaliy Makkoveev <[hidden email]> wrote:

>
>
>
> > On 29 Jul 2020, at 00:09, sven falempin <[hidden email]> wrote:
> >
> > On Tue, Jul 28, 2020 at 4:42 PM Vitaliy Makkoveev <[hidden email]> wrote:
> >>
> >> On Tue, Jul 28, 2020 at 04:10:01PM -0400, sven falempin wrote:
> >>> Hello,
> >>>
> >>> I am running some trunk interfaces in a multi core environment,
> >>> it's a slightly modified version, i have a few NET_ASSERT_LOCKED();
> >>> suspecting some multi core shenanigans, which i guess was confirmed:
> >>> (unsure the have X meaning, but i ' m pretty sure 256 is very wrong)
> >>> the if_trunk.c locking is completely unmodified
> >>> The code is 6.7-stable
> >>>
> >>> splassert: lacp_detach: want 2 have 0
> >>> splassert: lacp_detach: want 2 have 0
> >>> splassert: lacp_detach: want 2 have 256
> >>>
> >>> I noticed : trunk_clone_destroy ,call
> >>>
> >>>    if (tr->tr_proto != TRUNK_PROTO_NONE)
> >>>        tr->tr_detach(tr);
> >>>
> >>> outside the lock, and that trunk_ioctl call it
> >>>
> >>>        if (tr->tr_proto != TRUNK_PROTO_NONE)
> >>>            error = tr->tr_detach(tr);
> >>>
> >>> but ioctl is as far as i understand locked.
> >>> I'm unsure if the difficult and amazing unlocking work
> >>> did an oopsies or if ioctl is already assumed unlocked.
> >>>
> >>> Kindly inform me.
> >>> Best regards, thank you for reading.
> >>>
> >>
> >> lacp_detach() touches nothing which requires NET_LOCK(). What is the
> >> reason you placed assertion to lacp_detach()?
> >
> > <<it's a slightly modified version, i have a few NET_ASSERT_LOCKED()>>
> >
> > the lacp_detach is not yours, i put them here because i have a NULL pointer
> > popping in other 'driver' callback.
> >
> > I'm tracking this and trying to understand  how this memory is 'nullified'
> > mid function.
>
> I have no telepathy skills. Sorry. If you have panics send please dmesg(8)
> output and instruction for reproduce.
>
> >
> > I do not think putting NET_ASSERT_LOCKED can be harmful in any way.
> > If so please tell me.
> >
>
> What is the data you think needs be protected by netlock?
>
> > I am just tracking  a bug  and noticed these detach locking strangeness.
> >
>


After further analysis the *current* conclusion is:
 * current code is sort of safe, but the delete would be safer with a
lock ( for consistency anyway )
 * if the code run into a VM that do strange stuff with the stack it
creates bugs ( the bug only appears in KVM cpu ,
a qemu emulating a real cpu does not have this problem )

--
--
---------------------------------------------------------------------------------------------------------------------
Knowing is not enough; we must apply. Willing is not enough; we must do