Changeset View
Standalone View
sys/kern/vfs_syscalls.c
Context not available. | |||||
sys_unlink(struct thread *td, struct unlink_args *uap) | sys_unlink(struct thread *td, struct unlink_args *uap) | ||||
{ | { | ||||
return (kern_unlinkat(td, AT_FDCWD, uap->path, UIO_USERSPACE, 0)); | return (kern_fdunlinkat(td, AT_FDCWD, uap->path, FD_NONE, UIO_USERSPACE, | ||||
0)); | |||||
} | |||||
static int | |||||
kern_fdunlinkat_ex(struct thread *td, int dfd, const char *path, int fd, | |||||
int flag, enum uio_seg pathseg, ino_t oldinum) | |||||
{ | |||||
if ((flag & ~AT_REMOVEDIR) != 0) | |||||
kib: if ((flag & ~AT_REMOVEDIR) != 0) | |||||
return (EINVAL); | |||||
if ((flag & AT_REMOVEDIR) != 0) | |||||
Done Inline ActionsSame. kib: Same. | |||||
return (kern_fdrmdirat(td, dfd, path, fd, UIO_USERSPACE)); | |||||
return (kern_fdunlinkat(td, dfd, path, fd, UIO_USERSPACE, 0)); | |||||
} | } | ||||
#ifndef _SYS_SYSPROTO_H_ | #ifndef _SYS_SYSPROTO_H_ | ||||
Context not available. | |||||
int | int | ||||
sys_unlinkat(struct thread *td, struct unlinkat_args *uap) | sys_unlinkat(struct thread *td, struct unlinkat_args *uap) | ||||
{ | { | ||||
int flag = uap->flag; | |||||
int fd = uap->fd; | |||||
char *path = uap->path; | |||||
if (flag & ~AT_REMOVEDIR) | return (kern_fdunlinkat_ex(td, uap->fd, uap->path, FD_NONE, uap->flag, | ||||
return (EINVAL); | UIO_USERSPACE, 0)); | ||||
Done Inline ActionsPointer requires larger alignment than int, put mp declaration before error. kib: Pointer requires larger alignment than int, put mp declaration before error. | |||||
} | |||||
if (flag & AT_REMOVEDIR) | #ifndef _SYS_SYSPROTO_H_ | ||||
return (kern_rmdirat(td, fd, path, UIO_USERSPACE)); | struct fdunlinkat_args { | ||||
else | int dfd; | ||||
return (kern_unlinkat(td, fd, path, UIO_USERSPACE, 0)); | const char *path; | ||||
int fd; | |||||
int flag; | |||||
Done Inline Actionsreturn(EBUSY); The 'else' block than can be unindented. kib: return(EBUSY);
The 'else' block than can be unindented. | |||||
}; | |||||
#endif | |||||
int | |||||
sys_fdunlinkat(struct thread *td, struct fdunlinkat_args *uap) | |||||
{ | |||||
return (kern_fdunlinkat_ex(td, uap->dfd, uap->path, uap->fd, uap->flag, | |||||
UIO_USERSPACE, 0)); | |||||
} | } | ||||
int | int | ||||
kern_unlinkat(struct thread *td, int fd, char *path, enum uio_seg pathseg, | kern_fdunlinkat(struct thread *td, int dfd, const char *path, int fd, | ||||
Not Done Inline ActionsERESTART looks more suitable for this hack, if not the bigger problem. Above call to vn_start_write() specifies PCATCH, which makes it possible for signal to cause EINTR/ERESTART error returned from vn_start_write(). Then instead of aborting the op, you re-enter the function and loop this way until suspension on fs is lifted. kib: ERESTART looks more suitable for this hack, if not the bigger problem. Above call to… | |||||
Not Done Inline ActionsBoth EINTR and ERESTART would be wrong here, since once such an error has been returned by a sleep with PCATCH, any sleep with PCATCH will immediately return that error until the signal has been handled. Therefore such errors need to be returned to the caller. The restart in unlinking needs a different return value, possibly not an error number. jilles: Both `EINTR` and `ERESTART` would be wrong here, since once such an error has been returned by… | |||||
Not Done Inline ActionsThis is exactly what I tried to note. kib: This is exactly what I tried to note. | |||||
ino_t oldinum) | enum uio_seg pathseg, ino_t oldinum) | ||||
{ | { | ||||
struct mount *mp; | struct mount *mp; | ||||
struct file *fp; | |||||
struct vnode *vp; | struct vnode *vp; | ||||
struct nameidata nd; | struct nameidata nd; | ||||
struct stat sb; | struct stat sb; | ||||
cap_rights_t rights; | cap_rights_t rights; | ||||
int error; | int error; | ||||
Done Inline ActionsSame comment about setting fp to NULL and checking fp != NULL instead of fd != -1 on fdout. kib: Same comment about setting fp to NULL and checking fp != NULL instead of fd != -1 on fdout. | |||||
fp = NULL; | |||||
if (fd != FD_NONE) { | |||||
error = getvnode(td, fd, cap_rights_init(&rights, CAP_LOOKUP), | |||||
&fp); | |||||
if (error != 0) | |||||
return (error); | |||||
} | |||||
restart: | restart: | ||||
bwillwrite(); | bwillwrite(); | ||||
NDINIT_ATRIGHTS(&nd, DELETE, LOCKPARENT | LOCKLEAF | AUDITVNODE1, | NDINIT_ATRIGHTS(&nd, DELETE, LOCKPARENT | LOCKLEAF | AUDITVNODE1, | ||||
pathseg, path, fd, cap_rights_init(&rights, CAP_UNLINKAT), td); | pathseg, path, dfd, cap_rights_init(&rights, CAP_UNLINKAT), td); | ||||
if ((error = namei(&nd)) != 0) | if ((error = namei(&nd)) != 0) { | ||||
return (error == EINVAL ? EPERM : error); | if (error == EINVAL) | ||||
Done Inline ActionsI think if (error == EINVAL) error = EPERM; is cleaner. kib: I think
```
if (error == EINVAL)
error = EPERM;
```
is cleaner. | |||||
error = EPERM; | |||||
goto fdout; | |||||
} | |||||
vp = nd.ni_vp; | vp = nd.ni_vp; | ||||
if (vp->v_type == VDIR && oldinum == 0) { | if (vp->v_type == VDIR && oldinum == 0) { | ||||
error = EPERM; /* POSIX */ | error = EPERM; /* POSIX */ | ||||
} else if (oldinum != 0 && | } else if (oldinum != 0 && | ||||
((error = vn_stat(vp, &sb, td->td_ucred, NOCRED, td)) == 0) && | ((error = vn_stat(vp, &sb, td->td_ucred, NOCRED, td)) == 0) && | ||||
sb.st_ino != oldinum) { | sb.st_ino != oldinum) { | ||||
error = EIDRM; /* Identifier removed */ | error = EIDRM; /* Identifier removed */ | ||||
Done Inline ActionsSame as for fdrmdirat, use fp->f_vnode directly there. kib: Same as for fdrmdirat, use fp->f_vnode directly there. | |||||
} else if (fp != NULL && fp->f_vnode != vp) { | |||||
error = EAGAIN; | |||||
} else { | } else { | ||||
/* | /* | ||||
* The root of a mounted filesystem cannot be deleted. | * The root of a mounted filesystem cannot be deleted. | ||||
Not Done Inline ActionsThere is a corner situation possible which might deserve more delicate handling. Since fp->f_vnode is not locked while the reference is held, it is possible for the f_vnode to be reclaimed, and then namei() call above would allocate the new vnode for the same inode. So despite user specified correct file descriptor, the condition is not true and we return EDEADLK. IMO, you should check the VI_DOOMED flag and return EBADF instead of EDEADLK if f_vnode != vp and VI_DOOMED and set. kib: There is a corner situation possible which might deserve more delicate handling. Since fp… | |||||
Context not available. | |||||
else | else | ||||
vput(vp); | vput(vp); | ||||
if ((error = vn_start_write(NULL, &mp, | if ((error = vn_start_write(NULL, &mp, | ||||
V_XSLEEP | PCATCH)) != 0) | V_XSLEEP | PCATCH)) != 0) { | ||||
return (error); | goto fdout; | ||||
} | |||||
goto restart; | goto restart; | ||||
} | } | ||||
Done Inline ActionsThis is too old story. I do not see much point in sysproto copy/paste today. kib: This is too old story. I do not see much point in sysproto copy/paste today. | |||||
#ifdef MAC | #ifdef MAC | ||||
Done Inline ActionsNo initialization in declarations, style(9). Also, the values are used only once, so why do you need locals at all ? kib: No initialization in declarations, style(9). Also, the values are used only once, so why do… | |||||
Context not available. | |||||
vrele(vp); | vrele(vp); | ||||
else | else | ||||
vput(vp); | vput(vp); | ||||
fdout: | |||||
if (fp != NULL) | |||||
Done Inline ActionsUnneeded {}. kib: Unneeded {}. | |||||
fdrop(fp, td); | |||||
return (error); | return (error); | ||||
} | } | ||||
Not Done Inline ActionsHow can error be non-zero at this point ? kib: How can error be non-zero at this point ? | |||||
Context not available. | |||||
AUDIT_ARG_FD(uap->fd); | AUDIT_ARG_FD(uap->fd); | ||||
AUDIT_ARG_FFLAGS(uap->flags); | AUDIT_ARG_FFLAGS(uap->flags); | ||||
error = getvnode(td, uap->fd, cap_rights_init(&rights, CAP_FCHFLAGS), | error = getvnode(td, uap->fd, cap_rights_init(&rights, CAP_LOOKUP), | ||||
&fp); | &fp); | ||||
if (error != 0) | if (error != 0) | ||||
return (error); | return (error); | ||||
Context not available. | |||||
sys_rmdir(struct thread *td, struct rmdir_args *uap) | sys_rmdir(struct thread *td, struct rmdir_args *uap) | ||||
{ | { | ||||
return (kern_rmdirat(td, AT_FDCWD, uap->path, UIO_USERSPACE)); | return (kern_fdrmdirat(td, AT_FDCWD, uap->path, FD_NONE, | ||||
UIO_USERSPACE)); | |||||
} | } | ||||
int | int | ||||
kern_rmdirat(struct thread *td, int fd, char *path, enum uio_seg pathseg) | kern_fdrmdirat(struct thread *td, int dfd, const char *path, int fd, | ||||
enum uio_seg pathseg) | |||||
{ | { | ||||
struct mount *mp; | struct mount *mp; | ||||
struct vnode *vp; | struct vnode *vp; | ||||
struct file *fp; | |||||
struct nameidata nd; | struct nameidata nd; | ||||
cap_rights_t rights; | cap_rights_t rights; | ||||
int error; | int error; | ||||
Done Inline ActionsSet fp to NULL there. kib: Set fp to NULL there. | |||||
fp = NULL; | |||||
if (fd != FD_NONE) { | |||||
error = getvnode(td, fd, cap_rights_init(&rights, CAP_LOOKUP), | |||||
&fp); | |||||
if (error != 0) | |||||
return (error); | |||||
} | |||||
restart: | restart: | ||||
bwillwrite(); | bwillwrite(); | ||||
NDINIT_ATRIGHTS(&nd, DELETE, LOCKPARENT | LOCKLEAF | AUDITVNODE1, | NDINIT_ATRIGHTS(&nd, DELETE, LOCKPARENT | LOCKLEAF | AUDITVNODE1, | ||||
pathseg, path, fd, cap_rights_init(&rights, CAP_UNLINKAT), td); | pathseg, path, dfd, cap_rights_init(&rights, CAP_UNLINKAT), td); | ||||
if ((error = namei(&nd)) != 0) | if ((error = namei(&nd)) != 0) | ||||
return (error); | goto fdout; | ||||
vp = nd.ni_vp; | vp = nd.ni_vp; | ||||
if (vp->v_type != VDIR) { | if (vp->v_type != VDIR) { | ||||
error = ENOTDIR; | error = ENOTDIR; | ||||
Context not available. | |||||
error = EBUSY; | error = EBUSY; | ||||
goto out; | goto out; | ||||
} | } | ||||
Done Inline ActionsYou only use vfdp there, IMO it would be cleaner to do if (fp != NULL && fp->f_vnode != vp) kib: You only use vfdp there, IMO it would be cleaner to do
```
if (fp != NULL && fp->f_vnode != vp)… | |||||
if (fp != NULL && fp->f_vnode != vp) { | |||||
error = EAGAIN; | |||||
goto out; | |||||
} | |||||
#ifdef MAC | #ifdef MAC | ||||
error = mac_vnode_check_unlink(td->td_ucred, nd.ni_dvp, vp, | error = mac_vnode_check_unlink(td->td_ucred, nd.ni_dvp, vp, | ||||
&nd.ni_cnd); | &nd.ni_cnd); | ||||
Context not available. | |||||
else | else | ||||
vput(nd.ni_dvp); | vput(nd.ni_dvp); | ||||
if ((error = vn_start_write(NULL, &mp, V_XSLEEP | PCATCH)) != 0) | if ((error = vn_start_write(NULL, &mp, V_XSLEEP | PCATCH)) != 0) | ||||
return (error); | goto fdout; | ||||
goto restart; | goto restart; | ||||
} | } | ||||
vfs_notify_upper(vp, VFS_NOTIFY_UPPER_UNLINK); | vfs_notify_upper(vp, VFS_NOTIFY_UPPER_UNLINK); | ||||
Not Done Inline ActionsSame note about VI_DOOMED there. kib: Same note about VI_DOOMED there. | |||||
Context not available. | |||||
vrele(nd.ni_dvp); | vrele(nd.ni_dvp); | ||||
else | else | ||||
vput(nd.ni_dvp); | vput(nd.ni_dvp); | ||||
fdout: | |||||
Done Inline Actionsif (fp != NULL) fdrop(fp, td); kib: ```
if (fp != NULL)
fdrop(fp, td);
``` | |||||
if (fp != NULL) | |||||
fdrop(fp, td); | |||||
return (error); | return (error); | ||||
} | } | ||||
Context not available. |
if ((flag & ~AT_REMOVEDIR) != 0)