Page MenuHomeFreeBSD

unionfs: rework pathname handling
ClosedPublic

Authored by jah on Aug 30 2021, 3:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jun 5, 2:22 AM
Unknown Object (File)
Mar 29 2024, 2:16 AM
Unknown Object (File)
Dec 20 2023, 1:00 AM
Unknown Object (File)
Dec 14 2023, 8:36 PM
Unknown Object (File)
Nov 28 2023, 10:28 AM
Unknown Object (File)
Nov 25 2023, 3:00 AM
Unknown Object (File)
Nov 22 2023, 6:37 PM
Unknown Object (File)
Nov 22 2023, 10:22 AM
Subscribers

Details

Summary

Running stress2 unionfs tests reliably produces a namei_zone corruption
panic due to unionfs_relookup() attempting to NUL-terminate a newly-
allocate pathname buffer without first validating the buffer length.

Instead, avoid allocating new pathname buffers in unionfs entirely,
using already-provided buffers while ensuring the the correct flags
are set in struct componentname to prevent freeing or manipulation
of those buffers at lower layers.

While here, also compute and store the path length once in the unionfs
node instead of constantly invoking strlen() on it.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jah requested review of this revision.Aug 30 2021, 3:07 PM

Based on my reading of namei(9) and the code, it seems like what I'm doing here (directly passing the pathname buffer) should be OK.
During testing I also instrumented the unionfs code to verify that the lower layers weren't altering the contents of cn_pnbuf. But being new to namei, I also wouldn't be surprised if this approach is problematic for reasons that aren't apparent to me. If so, I'll go back to allocating duplicate buffers for the unionfs lookup.

sys/fs/unionfs/union_subr.c
858

Fix style while here?

sys/fs/unionfs/union_vnops.c
1017

Is this a logic change? Before we'd set path = NULL in one case - was that a bug?

sys/fs/unionfs/union_vnops.c
1017

From what I could tell that was just dead logic. Before this change, unionfs_mkwhiteout (which is the only thing that consumes 'path' here) would just use cnp->cn_nameptr if the passed-in path was NULL. But also, we were only setting path to NULL for the case in which the remove operation would target the upper vnode, which doesn't produce a call to unionfs_mkwhiteout() in the first place since a plain VOP_REMOVE() for the upper FS should be fine.

kib added inline comments.
sys/fs/unionfs/union_subr.c
332

I would fix style for this statement, even if as a preliminary commit.
No need to cast, + 1, M_WAITOK | M_ZERO.

604

() != 0

This revision is now accepted and ready to land.Aug 30 2021, 11:26 PM
This revision was automatically updated to reflect the committed changes.