Page MenuHomeFreeBSD
Differential D18359 Diff 51220 sys/kern/vfs_syscalls.c

Changeset View

Standalone View

View Options

sys/kern/vfs_syscalls.c

Context not available.
int int
sys_lgetfh(struct thread *td, struct lgetfh_args *uap) sys_lgetfh(struct thread *td, struct lgetfh_args *uap)
{ {
brooksUnsubmitted
Done
Inline Actions

Blank line after nonexistent local variables.

brooks: Blank line after nonexistent local variables.
int error = kern_getfhat(td, AT_SYMLINK_NOFOLLOW, AT_FDCWD, uap->fname,
UIO_USERSPACE, uap->fhp);
brooksUnsubmitted
Done
Inline Actions

This might be a phabricator issue, but it looks like this is indented with 2 tabs rather than a tab and four spaces.

brooks: This might be a phabricator issue, but it looks like this is indented with 2 tabs rather than a…
return (error);
}
#ifndef _SYS_SYSPROTO_H_
struct getfh_args {
char *fname;
fhandle_t *fhp;
};
#endif
int
sys_getfh(struct thread *td, struct getfh_args *uap)
{
int error = kern_getfhat(td, 0, AT_FDCWD, uap->fname,
kibUnsubmitted
Done
Inline Actions

Do not put initialization into local vars declaration.

That said, error var is pointless, just do return (kern_getfhat());.

kib: Do not put initialization into local vars declaration. That said, error var is pointless, just…
UIO_USERSPACE, uap->fhp);
return (error);
}
kibUnsubmitted
Not Done
Inline Actions

Blank line is needed after '{' since there are no local vars.

kib: Blank line is needed after '{' since there are no local vars.
/*
* syscall for the rpc.lockd to use to translate an open descriptor into
* a NFS file handle.
*
* warning: do not remove the priv_check() call or this becomes one giant
* security hole.
*/
#ifndef _SYS_SYSPROTO_H_
struct getfhat_args {
int fd;
char *path;
fhandle_t *fhp;
int flags;
};
#endif
int
sys_getfhat(struct thread *td, struct getfhat_args *uap)
{
if ((uap->flags & ~(AT_SYMLINK_NOFOLLOW | AT_BENEATH)) != 0)
kibUnsubmitted
Done
Inline Actions

There must be a blank line after '{' if there is no locals.

kib: There must be a blank line after '{' if there is no locals.
return (EINVAL);
return (kern_getfhat(td, uap->flags, uap->fd, uap->path,
UIO_USERSPACE, uap->fhp));
kibUnsubmitted
Done
Inline Actions

Continuation line should use +4 spaces.

kib: Continuation line should use +4 spaces.
}
int
brooksUnsubmitted
Done
Inline Actions

This function can be static.

brooks: This function can be static.
kibUnsubmitted
Not Done
Inline Actions

Wrong indent. Please review all the code for style issues that were already pointed out.

kib: Wrong indent. Please review all the code for style issues that were already pointed out.
kern_getfhat(struct thread *td, int flags, int fd, const char *path,
enum uio_seg pathseg, fhandle_t *fhp)
{
struct nameidata nd; struct nameidata nd;
fhandle_t fh; fhandle_t fh;
struct vnode *vp; struct vnode *vp;
cap_rights_t rights;
int error; int error;
error = priv_check(td, PRIV_VFS_GETFH); error = priv_check(td, PRIV_VFS_GETFH);
if (error != 0) if (error != 0)
return (error); return (error);
NDINIT(&nd, LOOKUP, NOFOLLOW | LOCKLEAF | AUDITVNODE1, UIO_USERSPACE,
uap->fname, td); if (path) {
kibUnsubmitted
Done
Inline Actions

if (path != NULL)

kib: if (path != NULL)
error = namei(&nd); NDINIT_AT(&nd, LOOKUP, (flags & AT_SYMLINK_NOFOLLOW ? NOFOLLOW : FOLLOW)
if (error != 0) | (flags & AT_BENEATH ? BENEATH : 0)
return (error); | LOCKLEAF | AUDITVNODE1,
NDFREE(&nd, NDF_ONLY_PNBUF); pathseg, path, fd, td);
brooksUnsubmitted
Done
Inline Actions

This indentation doesn't look like it follows style(9)

brooks: This indentation doesn't look like it follows style(9)
vp = nd.ni_vp;
kibUnsubmitted
Not Done
Inline Actions

Put the binary op designator before the split.

flags & AT_BENEATH is not boolean, use (flags & AT_BENEATH) != 0. Same for AT_SYMLINK_NOFOLLOW.

kib: Put the binary op designator before the split. flags & AT_BENEATH is not boolean, use `(flags…
error = namei(&nd);
if (error != 0)
return (error);
NDFREE(&nd, NDF_ONLY_PNBUF);
vp = nd.ni_vp;
}
brooksUnsubmitted
Done
Inline Actions

No line break

brooks: No line break
else {
error = fgetvp(td, fd, cap_rights_init(&rights, CAP_PREAD), &vp);
if (error != 0)
return (error);
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
kibUnsubmitted
Not Done
Inline Actions

LR_RETRY is allowed to return doomed vnode. You cannot access v_mount or call any VOP method on it.

That said, I find having the path == NULL case allowed for userspace somewhat strange. We do not allow NULL path for open(2), and we have getfh(2). I understand that this case allows to implement sys_gethf(), I am more about checking for path == NULL in getfhat().

kib: LR_RETRY is allowed to return doomed vnode. You cannot access v_mount or call any VOP method…
jack_gandi.netAuthorUnsubmitted
Done
Inline Actions

I agree the path = NULL case shouldn't be allowed for getfh, so I moved the logic into sys_getfhat.

jack_gandi.net: I agree the path = NULL case shouldn't be allowed for `getfh`, so I moved the logic into…
}
bzero(&fh, sizeof(fh)); bzero(&fh, sizeof(fh));
fh.fh_fsid = vp->v_mount->mnt_stat.f_fsid; fh.fh_fsid = vp->v_mount->mnt_stat.f_fsid;
error = VOP_VPTOFH(vp, &fh.fh_fid); error = VOP_VPTOFH(vp, &fh.fh_fid);
vput(vp); vput(vp);
if (error == 0) if (error == 0)
error = copyout(&fh, uap->fhp, sizeof (fh)); error = copyout(&fh, fhp, sizeof (fh));
return (error); return (error);
} }
#ifndef _SYS_SYSPROTO_H_ #ifndef _SYS_SYSPROTO_H_
struct getfh_args { struct fhlink_args {
char *fname; fhandle_t *fhp;
fhandle_t *fhp; int tofd;
const char *to;
}; };
#endif #endif
int sys_fhlink(struct thread *td, struct fhlink_args *uap)
brooksUnsubmitted
Done
Inline Actions

Newline before function name.

brooks: Newline before function name.
{
return (kern_fhlinkat(td, AT_FDCWD, uap->to, UIO_USERSPACE, uap->fhp, 0));
}
kibUnsubmitted
Not Done
Inline Actions

Again blank line after '{'.

kib: Again blank line after '{'.
#ifndef _SYS_SYSPROTO_H_
struct fhlinkat_args {
fhandle_t *fhp;
int tofd;
const char *to;
int flags;
};
#endif
int sys_fhlinkat(struct thread *td, struct fhlinkat_args *uap)
{
if ((uap->flags & ~(AT_SYMLINK_FOLLOW | AT_BENEATH)) != 0)
return (EINVAL);
return (kern_fhlinkat(td, uap->flags, uap->to, UIO_USERSPACE, uap->fhp,
uap->flags));
}
int kern_fhlinkat(struct thread *td, int fd, const char *path,
brooksUnsubmitted
Done
Inline Actions

This can be static.

brooks: This can be static.
enum uio_seg pathseg, fhandle_t *fhp, int flags)
{
fhandle_t fh;
struct mount *mp;
struct vnode *vp;
struct nameidata nd;
int error;
error = priv_check(td, PRIV_VFS_GETFH);
if (error != 0)
return (error);
error = copyin(fhp, &fh, sizeof(fh));
if (error != 0)
return (error);
again:
bwillwrite();
if ((mp = vfs_busyfs(&fh.fh_fsid)) == NULL)
return (ESTALE);
error = VFS_FHTOVP(mp, &fh.fh_fid, LK_EXCLUSIVE, &vp);
vfs_unbusy(mp);
if (error != 0)
return (error);
/* code taken from kern_linkat */
kibUnsubmitted
Done
Inline Actions

This comment is pointless. Either create a helper function used by both, or remove it.

kib: This comment is pointless. Either create a helper function used by both, or remove it.
kibUnsubmitted
Not Done
Inline Actions

So I actually mean that duplication of the code is wrong, and I really do not see why do we need two copies. kern_linkat() after the first namei() is indeed exact copy, so why not add the helper ?

kib: So I actually mean that duplication of the code is wrong, and I really do not see why do we…
jack_gandi.netAuthorUnsubmitted
Done
Inline Actions

I've added a helper function, the code was not _exactly_ the same so I had to change the tag/goto into a do while/EAGAIN, also has to add a locked variable. If these changes are ok by you I think it's much cleaner now.

jack_gandi.net: I've added a helper function, the code was not _exactly_ the same so I had to change the…
if (vp->v_type == VDIR) {
vput(vp);
return (EPERM); /* POSIX */
}
/* reuse the same target rights capability as linkat() */
NDINIT_ATRIGHTS(&nd, CREATE, (flags & AT_SYMLINK_NOFOLLOW ? NOFOLLOW : FOLLOW)
| (flags & AT_BENEATH ? BENEATH : 0)
| LOCKPARENT | SAVENAME | AUDITVNODE2 | NOCACHE,
pathseg, path, fd, &cap_linkat_target_rights, td);
if ((error = namei(&nd)) == 0) {
kibUnsubmitted
Not Done
Inline Actions

Stray blank line, more such lines below.

kib: Stray blank line, more such lines below.
if (nd.ni_vp != NULL) {
NDFREE(&nd, NDF_ONLY_PNBUF);
if (nd.ni_dvp == nd.ni_vp)
vrele(nd.ni_dvp);
else
vput(nd.ni_dvp);
vrele(nd.ni_vp);
vput(vp);
return (EEXIST);
} else if (nd.ni_dvp->v_mount != vp->v_mount) {
/*
* Cross-device link. No need to recheck
* vp->v_type, since it cannot change, except
* to VBAD.
*/
NDFREE(&nd, NDF_ONLY_PNBUF);
vput(nd.ni_dvp);
vput(vp);
return (EXDEV);
} else {
error = can_hardlink(vp, td->td_ucred);
#ifdef MAC
if (error == 0)
error = mac_vnode_check_link(td->td_ucred,
nd.ni_dvp, vp, &nd.ni_cnd);
#endif
if (error != 0) {
vput(vp);
vput(nd.ni_dvp);
NDFREE(&nd, NDF_ONLY_PNBUF);
return (error);
}
error = vn_start_write(vp, &mp, V_NOWAIT);
kibUnsubmitted
Not Done
Inline Actions

Remove all blank lines from the function starting with this one, they are not needed/allowed by style.

kib: Remove all blank lines from the function starting with this one, they are not needed/allowed by…
if (error != 0) {
kibUnsubmitted
Not Done
Inline Actions

Why exclusive lock ?

kib: Why exclusive lock ?
jack_gandi.netAuthorUnsubmitted
Done
Inline Actions

you're right, readlinkat(2) does LOCKSHARED . behaviour should be the same with`fhreadlinkat`

jack_gandi.net: you're right, `readlinkat(2)` does LOCKSHARED . behaviour should be the same with`fhreadlinkat`
vput(vp);
vput(nd.ni_dvp);
NDFREE(&nd, NDF_ONLY_PNBUF);
error = vn_start_write(NULL, &mp,
V_XSLEEP | PCATCH);
if (error != 0)
return (error);
goto again;
}
error = VOP_LINK(nd.ni_dvp, vp, &nd.ni_cnd);
vput(nd.ni_dvp);
vn_finished_write(mp);
NDFREE(&nd, NDF_ONLY_PNBUF);
}
}
vput(vp);
return (error);
}
#ifndef _SYS_SYSPROTO_H_
struct fhreadlink_args {
fhandle_t *fhp;
char *buf;
size_t bufsize;
};
#endif
int int
sys_getfh(struct thread *td, struct getfh_args *uap) sys_fhreadlink(struct thread *td, struct fhreadlink_args *uap)
{ {
struct nameidata nd;
fhandle_t fh; fhandle_t fh;
struct iovec aiov;
struct uio auio;
struct mount *mp;
struct vnode *vp; struct vnode *vp;
int error; int error;
Context not available.
error = priv_check(td, PRIV_VFS_GETFH); error = priv_check(td, PRIV_VFS_GETFH);
if (error != 0) if (error != 0)
return (error); return (error);
NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF | AUDITVNODE1, UIO_USERSPACE,
uap->fname, td); if (uap->bufsize > IOSIZE_MAX)
error = namei(&nd); return (EINVAL);
error = copyin(uap->fhp, &fh, sizeof(fh))
if (error != 0) if (error != 0)
return (error); return (error);
NDFREE(&nd, NDF_ONLY_PNBUF);
vp = nd.ni_vp; if ((mp = vfs_busyfs(&fh.fh_fsid)) == NULL)
bzero(&fh, sizeof(fh)); return (ESTALE);
fh.fh_fsid = vp->v_mount->mnt_stat.f_fsid;
error = VOP_VPTOFH(vp, &fh.fh_fid); error = VFS_FHTOVP(mp, &fh.fh_fid, LK_EXCLUSIVE, &vp);
vfs_unbusy(mp);
if (error != 0)
return (error);
#ifdef MAC
error = mac_vnode_check_readlink(td->td_ucred, vp);
if (error != 0) {
vput(vp);
return (error);
kibUnsubmitted
Not Done
Inline Actions

Same question there, IMO it is worth introducing the helper and avoid copy/paste.

kib: Same question there, IMO it is worth introducing the helper and avoid copy/paste.
}
#endif
if (vp->v_type != VLNK && (vp->v_vflag & VV_READLINK) == 0)
error = EINVAL;
else {
aiov.iov_base = uap->buf;
aiov.iov_len = uap->bufsize;
auio.uio_iov = &aiov;
auio.uio_iovcnt = 1;
auio.uio_offset = 0;
auio.uio_rw = UIO_READ;
auio.uio_segflg = UIO_USERSPACE;
auio.uio_td = td;
auio.uio_resid = uap->bufsize;
error = VOP_READLINK(vp, &auio, td->td_ucred);
td->td_retval[0] = uap->bufsize - auio.uio_resid;
}
vput(vp); vput(vp);
if (error == 0)
error = copyout(&fh, uap->fhp, sizeof (fh));
return (error); return (error);
} }
Context not available.