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.

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rrs created this revision.Jan 30 2019, 1:43 PM

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

rrs added a comment.Jan 30 2019, 4:31 PM

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

rrs added a comment.Jan 31 2019, 8:07 PM

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.

rrs added a comment.Feb 1 2019, 12:18 PM

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

rrs updated this revision to Diff 53505.Feb 1 2019, 12:31 PM

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

hselasky added inline comments.Feb 1 2019, 2:27 PM
if_lagg.c
521 ↗(On Diff #53505)

You have an extra chunk here?

rrs added inline comments.Feb 1 2019, 4:01 PM
if_lagg.c
521 ↗(On Diff #53505)

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

rrs added a comment.Feb 1 2019, 4:03 PM

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

rrs updated this revision to Diff 53514.Feb 1 2019, 4:04 PM

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

rrs added inline comments.Feb 1 2019, 4:05 PM
if_lagg.c
521 ↗(On Diff #53505)

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

Which is what it should be.

hselasky added inline comments.Feb 1 2019, 4:36 PM
if_lagg.c
521 ↗(On Diff #53505)

See:

https://reviews.freebsd.org/D19040

This chunk is no longer needed.

--HPS

rrs added inline comments.Feb 4 2019, 12:11 PM
if_lagg.c
521 ↗(On Diff #53505)

ahh Perfect :)

rrs updated this revision to Diff 53772.Feb 11 2019, 12:37 PM

get the update in and fix the pesky extra line.

hselasky accepted this revision.Feb 11 2019, 1:05 PM
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.