Open jharmening opened 10 months ago
lundman
commented
10 months ago Just so I understand,
zfs_vnop_mkdir() is called with the OS side parameters - whatever they may be, which will include struct componentname and cnp->cn_namelen as part of the wrapper layer (fbsd, macos, window at least, I think zpl_file has something similar).
But then it calls zfs_mkdir() where it drops struct componentname parameter, and you propose we keep it?
jharmening
commented
10 months ago Well, I'm really proposing that zfs_mkdir (which appears to be shared between the Linux and FreeBSD implementations) be changed to accept the length of 'dirname' as an explicit parameter instead of obtaining it by calling strlen(). The FreeBSD implementation in zfs_freebsd_mkdir() would pass ap->a_cnp->cn_namelen for this new param.
It does seem as though a fair amount of plumbing work would be needed for this change: For example, zfs_dirent_lock() would need to take the length and use it to set dl->dl_namesize immediately while using some other means besides dl->dl_namesize == 0 to determine whether dl->dl_name has been copied. But (naively at least) it seems as though this could make things better overall, by eliminating the need to call strlen() in so many places and reducing the risk of buffer overflow in case someone fails to properly NUL-terminate.
System information
Describe the problem you're observing
zfs_mkdir() currently takes the name of the new directory as the 'dirname' parameter and obtains the length via strlen(). This is a potential security issue and is inefficient in cases for which the length is already known, but it also causes a functional issue with unionfs on FreeBSD: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=275871
Since unionfs can create shadow directories as part of its lookup operation, the struct componentname * it provides to VOP_MKDIR() may have come from vfs_lookup(), in which case cnp->cn_nameptr will contain the remaining portion of the entire lookup path, delimited by '/'. cnp->cn_namelen will, however, correctly reflect the length of only the current path component. But zfs_freebsd_mkdir() doesn't respect cnp->cn_namelen because zfs_mkdir() doesn't make that possible. This results in incorrectly-named directories containing "/" which breaks pathname resolution as described in the bug report.
Describe how to reproduce the problem
zfs create zroot/var/repro mkdir -p /var/repro/z/a echo > /var/repro/z/a/b mkdir -p /var/repro/y mount_unionfs /var/repro/y /var/repro/z echo "blah" > /var/repro/z/a/b
-> Observe the presence of a non-deletable file named "a/b" in /var/repro/y
While this can be worked around easily enough on the unionfs side of things, changing zfs_mkdir() to accept a length parameter seems like the better long-term fix.
Include any warning/errors/backtraces from the system logs