Balance the if_snd_tag_alloc with an appropriate if_snd_tag_free

[rrs]

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 :)

[• hselasky]

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]

In D19032#406766, @hselasky wrote:

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!

[• hselasky]

In D19032#406795, @rrs wrote:

In D19032#406766, @hselasky 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]

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)

[• hselasky]

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]

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

[rrs]

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

[• hselasky]

if_lagg.c

521 ↗

(On Diff #53505)

You have an extra chunk here?

[rrs]

if_lagg.c

521 ↗

(On Diff #53505)

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

[rrs]

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

[rrs]

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

[rrs]

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]

if_lagg.c

521 ↗

(On Diff #53505)

See:

https://reviews.freebsd.org/D19040

This chunk is no longer needed.

--HPS

[rrs]

if_lagg.c

521 ↗

(On Diff #53505)

ahh Perfect :)

[rrs]

get the update in and fix the pesky extra line.