Page MenuHomeFreeBSD

Balance the if_snd_tag_alloc with an appropriate if_snd_tag_free
ClosedPublic

Authored by rrs on Jan 30 2019, 1:43 PM.
Tags
None
Referenced Files
F80141565: D19032.diff
Thu, Mar 28, 12:17 PM
Unknown Object (File)
Mon, Mar 4, 2:37 PM
Unknown Object (File)
Mon, Mar 4, 2:37 PM
Unknown Object (File)
Mon, Mar 4, 2:36 PM
Unknown Object (File)
Jan 28 2024, 10:38 PM
Unknown Object (File)
Dec 23 2023, 3:06 AM
Unknown Object (File)
Dec 22 2023, 10:16 PM
Unknown Object (File)
Dec 11 2023, 3:27 AM
Subscribers

Details

Summary

Basically we have had crashes where rate limiting is used and
a lagg is in play. When the free is attempted (from within the tcp stack)
it causes a crash due to the fact that the source ifp (lagg) does not
have a balanced free routine.

Test Plan

I have tested this patch at NF by making the BBR and Rack stacks
actually use the free and not crash with this patch :)

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Hi,

This patch does not look correct. You will need to pass the flowid in order to correctly handle this inside lagg?

Why are you not using the tag->ifp->if_sndtag_free(tag) in the first place?

--HPS

Hi,

This patch does not look correct. You will need to pass the flowid in order to correctly handle this inside lagg?

Why are you not using the tag->ifp->if_sndtag_free(tag) in the first place?

--HPS

Hann's

Why in the world would I need the hash?

The tag itself has the ifp in it so the lagg can simply
reference this ifp to get to the correct interface.

There is no reason to access the hash, this would be just
extra book-keeping.

The interface that the transport sees is the ifp pointing to the lagg, not the
underlying interface. Which is how we crashed because the lagg is asymmetric in
that it did not properly support the free.

If it supports an allocate, it really needs to support a free.. and thats an easy thing
to do as seen above!

In D19032#406795, @rrs wrote:

Hi,

Hi,

The interface that the transport sees is the ifp pointing to the lagg, not the
underlying interface. Which is how we crashed because the lagg is asymmetric in
that it did not properly support the free.

The current implementation is not symmetric, but I don't object making it symmetric. However then you should set the default if_snd_tag_free() inside if_attach_internal() ?

If it supports an allocate, it really needs to support a free.. and thats an easy thing
to do as seen above!

--HPS

I don't follow your suggestion. Why would I add a if_snd_tag_free() in if_attach_internal() .
The driver below has to do that just like it has to add the if_snd_tag_alloc(). The
internal function has no way of knowing what is the right thing to do in
freeing a tag that the driver allocates. Having it be NULL as the default in
the internal just the same as if_snd_tag_alloc() is the right thing, only
if a driver sets these should they get used (when RATELIMIT is configured of course)

In D19032#407220, @rrs wrote:

I don't follow your suggestion. Why would I add a if_snd_tag_free() in if_attach_internal() .
The driver below has to do that just like it has to add the if_snd_tag_alloc(). The
internal function has no way of knowing what is the right thing to do in
freeing a tag that the driver allocates. Having it be NULL as the default in
the internal just the same as if_snd_tag_alloc() is the right thing, only
if a driver sets these should they get used (when RATELIMIT is configured of course)

OK, got you, but then you should also patch if_vlan.c , which also has a send tag alloc routine.

Ahh good catch I did not know vlan had an allocator.. I will add it to the patch-set

This updates the diff to Fix Hann's point on vlans, both the
vlan and lagg are now balanced.

if_lagg.c
521

You have an extra chunk here?

if_lagg.c
521

Ohh good catch.. must have been a mis-merge.

Interesting my svn copy does not have that extra chunk.. let me re-update the diff and use -x -up

Update to hopefully get a better view and not have the stray line in the diff

if_lagg.c
521

ok now its
#ifdef RATELIMIT
<3 lines including set the flags>
#else
<set the flags without TXRLMT>
#endif

Which is what it should be.

if_lagg.c
521

See:

https://reviews.freebsd.org/D19040

This chunk is no longer needed.

--HPS

if_lagg.c
521

ahh Perfect :)

get the update in and fix the pesky extra line.

This revision is now accepted and ready to land.Feb 11 2019, 1:05 PM
This revision was automatically updated to reflect the committed changes.