Page MenuHomeFreeBSD

Support unmapped mbufs in crypto buffers.
ClosedPublic

Authored by jhb on May 5 2021, 9:29 PM.

Details

Summary
  • Rename CRYPTO_MAY_HAVE_VMPAGE to CRYPTO_MAY_HAVE_DIRECT_MAP.
  • Make helper routines to support segment operations on unmapped mbufs conditional on CRYPTO_MAY_HAVE_DIRECT_MAP.

Sponsored by: Netflix

Diff Detail

Repository
R10 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

jhb requested review of this revision.May 5 2021, 9:29 PM
sys/opencrypto/criov.c
246

The indentation of the continuing line is off, ditto below.

257

Would be nice to have a local var pagelen = m_epg_pagelen(m, i, pgoff), ditto below.

260

I would KASSERT(offset < m_epg_trllen).

303

With commit 49f6925ca34 there is a good chance that the pages are physically contiguous, it may be profitable to check for that here.

311

The existence of this check implies that skip + len might be an offset past the end of the mbuf, but we appear to assume that skip alone is always within the bounds of the mbuf. It would be nice to have some assertions or comments documenting exactly what the assumptions are.

433

Just a general comment, I think it would be nicer and more efficient if we had a function which returned both the base address and length of the next segment. Many consumers call _segbase and _seglen back-to-back. It seems especially awkward with extpg buffers since we end up looping over the page array twice.

820

I don't really understand the ifdefs. On platforms without a direct map, PHYS_TO_DMAP() will panic. We don't have special ifdef guards around M_EXTPG handling elsewhere, so I'm not sure why it's required here.

sys/opencrypto/cryptodev.h
210 ↗(On Diff #88709)

Drop extra whitespace while here?

sys/opencrypto/criov.c
311

I was probably coding this a bit defensively. Looking at the caller, we check skip and len against m_len first and only invoke this if skip is < m_len. We probably should assert that.

433

Yes, that was not as apparent for the previous types, but the double loop is a bit much, and in practice it is true that callers tend to fetch the pair.

820

The m_epg_contiguous_subsegment() function is conditional on the #ifdef. I could perhaps always define the function instead and have it's body be #ifdef'ed?

sys/opencrypto/criov.c
820

I see. It's more that in generic code we handle M_EXTPG mbufs unconditionally, implicitly assuming that they won't arise on platforms without a direct map. I'd maybe move m_epg_contiguous_subsegment() out of the ifdef as well. I guess that has the downside of compiling dead code on platforms which don't have a direct map, so I don't have a strong preference. It's ok as-is.

jhb marked 6 inline comments as done.May 20 2021, 12:13 AM
jhb added inline comments.
sys/opencrypto/criov.c
820

I've removed them and also removed some explicit KASSERT()'s for HAS_DMAP counting on PHYS_TO_DMAP() to panic instead.

  • Various review feedback.
This revision is now accepted and ready to land.May 25 2021, 1:56 PM
This revision was automatically updated to reflect the committed changes.