iflib: Fix mbufs leaked by 0 len packets emitted from the if driver

[linnemannr_gmail.com]

Summary

Some interface drivers, notably bnxt, can insert 0 length packets onto
their receive queues when certain conditions are met, such as discarding
packets in the case of bnxt.

When this packet gets processed by assemble_segments(), The solitary
mbuf on the queue that composes it consist of a single zero length
fragment. The loop in assemble_segments() doesn't seem to expect
that a 0 length fragment can exist in the iri_frags list without a
non-zero length header preceding it. In this situation, without filter
intervention rxd_frag_to_sd() returns a pointer to the corresponding
mbuf in the rxq, where it is matched as a zero-length fragment and
immediately discarded without freeing as mh has not yet been assigned.

This change corrects this behavior by falling through the mh == NULL
case and freeing m on the condition that it is not NULL before
continuing the loop.

Sponsored by: Spectra Logic

[linnemannr_gmail.com]

I'm debating on whether we should also change rxd_frag_to_sd() to not pop the mbuf off the freelist and instead return NULL if irf->irf_len == 0. If the fragment is 0 length I don't think there is any point in consuming an mbuf over it, since it will just be immediately freed.

[kgalazka]

Shouldn't this be actually handled by this condition? (https://cgit.freebsd.org/src/tree/sys/net/iflib.c#n2969)

/* was this only a completion queue message? */
if (__predict_false(ri.iri_nfrags == 0))
	continue;

I mean if a packet is discarded does it need to use any iri_frags?

[linnemannr_gmail.com]

In D57537#1319074, @kgalazka wrote:

Shouldn't this be actually handled by this condition? (https://cgit.freebsd.org/src/tree/sys/net/iflib.c#n2969)

/* was this only a completion queue message? */
if (__predict_false(ri.iri_nfrags == 0))
	continue;

I mean if a packet is discarded does it need to use any iri_frags?

No, in the case we hit there was a packet of one fragment of zero length placed on the rxq. Ultimately there may be a bnxt bug, but the fact remains that it is logically possible to wind up at original line 2803 with mh == NULL and m != NULL, and we leak the mbuf as a result. That condition needs to be accounted for.

[gallatin]

It took me a while, but I think your patch is correct.

[kp]

In D57537#1319082, @gallatin wrote:

It took me a while, but I think your patch is correct.

@gallatin Do you want to commit this, or should I? (Reid doesn't have a bit, so someone else needs to.)

[gallatin]

In D57537#1321127, @kp wrote:

In D57537#1319082, @gallatin wrote:

It took me a while, but I think your patch is correct.

@gallatin Do you want to commit this, or should I? (Reid doesn't have a bit, so someone else needs to.)

If you could push it, that would be great. BTW, if you could look at https://reviews.freebsd.org/D57550 I'd appreciate it. I just added you, after realizing I added the wrong Kristof

[kp]

In D57537#1321192, @gallatin wrote:

@gallatin Do you want to commit this, or should I? (Reid doesn't have a bit, so someone else needs to.)

If you could push it, that would be great.

Will do.

BTW, if you could look at https://reviews.freebsd.org/D57550 I'd appreciate it. I just added you, after realizing I added the wrong Kristof

I'll take a look, but I'm not sure I'm the right Kristof either ;). I'm not particularly familiar with iflib.