Fix a NULL pointer reference in iflib

[rajesh1.kumar_amd.com]

Summary

rxd_frag_to_sd function can have pf_rv parameter as NULL with the current
code. This patch fixes the NULL pointer referce in that case thus avoiding a
possible panic.

[gallatin]

Whats the path to having a null pointer as pf_rv ?

[rajesh1.kumar_amd.com]

We faced this issue when testing the split header feature with "axgbe" driver.

For example, with split header enabled and Rx buffer as 4K. When peer sends a 9K packet, it will be received as 3 fragments as below (not the wire packet, but packet fragment from hardware to driver)

4K (header + data) + 4K (data alone) + 1K (data alone) roughly.

In axgbe case, each fragment uses 1 hardware descriptor with 2 buffer pointers (1 for header and 1 for data). These can be either two buffers from two freelists at same index, or two consecutive buffers from single freelist (based on driver design). In either case, rxd_pkt_get will return two iri_frags for each fragment.

Considering this case, when a packet has no header, but just data (second and third packet in above example). iri_frag 0 will have iri_len set to 0 (no header) and iri_frag will have iri_len set to the actual data len.

With this case and the current iflib code,

1. In "assemble_segments", the first call to rxd_frag_to_sd will pass a valid pointer in "pf_rv_ptr".
2. After the first call to "rxd_frag_to_sd", the following code will make pf_rv_ptr to NULL (since the first frag len is 0) and continue to second frag.

/*
                 * Exclude zero-length frags & frags from
                 * packets the filter has consumed or dropped
                 */
                if (ri->iri_frags[i].irf_len == 0 || consumed ||
                    *pf_rv == PFIL_CONSUMED || *pf_rv == PFIL_DROPPED) {
                        if (mh == NULL) {
                                /* everything saved here */
                                consumed = true;
                                pf_rv_ptr = NULL;
                                continue;
                        }
                        /* XXX we can save the cluster here, but not the mbuf */
                        m_init(m, M_NOWAIT, MT_DATA, 0);
                        m_free(m);
                        continue;
                }

3. When "rxd_frag_to_sd" is called for second frag, pf_rv_ptr is NULL and it takes the else path in "rxd_frag_to_sd", which leads to a NULL pointer access.

This patch is to ignore that NULL pointer access. Does this hold good, or should it be handled differently?

[gallatin]

OK, thanks that makes sense. I was not considering a multi-frag case.

This is embarrassing, but why doesn't axgbe just use a 9k jumbo buffer when the MTU is 9k? Does iflib not support that? Or the hardware? Or is it something else? Its generally much more efficient in the OS to use a single jumbo mbuf rather than chaining 3

[rajesh1.kumar_amd.com]

OK, thanks that makes sense. I was not considering a multi-frag case.

Thanks @gallatin for taking time to review and accept the patch.

This is embarrassing, but why doesn't axgbe just use a 9k jumbo buffer when the MTU is 9k? Does iflib not support that? Or the hardware? Or is it something else? Its generally much more efficient in the OS to use a single jumbo mbuf rather than chaining 3

It's not a hardware limitation. But it's set at the driver level. I believe this is to have a buffer size for both MTU 1500 and 9000 being statically preallocated during the driver init time itself. I shall look at for any other possible reason and if needed make appropriate changes.

But, looks like iflib also limits the buffer size to page size. Please correct me if my understanding is wrong.

static uint16_t
iflib_get_mbuf_size_for(unsigned int size)
{

        if (size <= MCLBYTES)
                return (MCLBYTES);
        else
                return (MJUMPAGESIZE);
}

where MJUMPAGESIZE is,

#if PAGE_SIZE < 2048
#define MJUMPAGESIZE    MCLBYTES
#elif PAGE_SIZE <= 8192
#define MJUMPAGESIZE    PAGE_SIZE
#else
#define MJUMPAGESIZE    (8 * 1024)
#endif

[rajesh1.kumar_amd.com]

@gallatin, I don't have the committer access for the git repo. Can you please help push this change upstream if there is no other comments?

[rajesh1.kumar_amd.com]

Thanks @gallatin for pushing the patch.