Re: Patch test & review: vop_stdaccess()
New version of patch incorporating most of the Brucifications: http://phk.freebsd.dk/patch/vaccess.patch Comments & tests please. -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD coreteam member | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Patch test & review: vop_stdaccess()
On Thu, 17 Aug 2000, Boris Popov wrote:
> On Wed, 16 Aug 2000, Poul-Henning Kamp wrote:
>
> > Please test and review this patch:
> >
> > http://phk.freebsd.dk/patch/vop_stdaccess.patch
>
> Looks fine to me except vop_stdaccess() itself.
vop_stdaccess() is badly named. It is not a vop function (one suitable
for putting in vop tables). In NetBSD, the corresponding function is:
int vaccess __P((enum vtype type, mode_t file_mode, uid_t uid, gid_t gid,
mode_t acc_mode, struct ucred *cred));
This is declared in and implemented in kern/vfs_subr.c.
It has the following interesting differences:
- it checks the X bits for root. Most or all of our filesystems omit this
check, so access(2) by root bogusly returns success for X_OK of files
with no X bits set, and VOP_ACCESS() can't actually be used to check for
X access by root. The POSIX spec for access(2) may permit this braindamage.
It permits access(2) for X_OK to succeed for processes with appropriate
privilege although no X bits are set. I'm not sure if it requires this
success to have anything to do with executability. execve() in the kernel
knows not to trust VOP_ACCESS() and does an explicit test of the X bits
(hard-coded as 0111 :-()) in check_permissions(). Shells know not to
trust access() and do similar explicit tests.
- it doesn't bother inlining groupmember().
- it is missing some style bugs.
The function could reasonably handle MNT_RDONLY, MNT_NOEXEC and
immutability, especially the first two since they are necessary for
most filesystems (cd9660 handles MNT_RDONLY although it doesn't support
writing). MNT_NOEXEC bogotifies access() in the oppisite direction
even for non-root. execve() in the kernel does an explicit check for
it. I guess shells and execlp() are confused by it.
> Since VREAD,
> VWRITE and VEXEC bits are carefully layed this function can be rewritten
> to use single shift operation instead of 3 'or's:
>
> if (cred->cr_uid != uid) {
> amode >>= 3;
> if (!groupmember(gid, cred))
> amode >>= 3;
> }
> return (fmode & amode) == amode ? 0 : EACCES;
This optimization seems to be intentionally left out. I don't think it
is worth doing. Hopefully the compiler will do it. gcc doesn't seem
to do it, but I've seen it generate good code for repacking less well
laid out bits in device drivers.
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Patch test & review: vop_stdaccess()
In message <[EMAIL PROTECTED]>, Boris Pop
ov writes:
>On Wed, 16 Aug 2000, Poul-Henning Kamp wrote:
>
>> Please test and review this patch:
>>
>> http://phk.freebsd.dk/patch/vop_stdaccess.patch
>
> Looks fine to me except vop_stdaccess() itself. Since VREAD,
>VWRITE and VEXEC bits are carefully layed this function can be rewritten
>to use single shift operation instead of 3 'or's:
>
> if (cred->cr_uid != uid) {
> amode >>= 3;
> if (!groupmember(gid, cred))
> amode >>= 3;
> }
> return (fmode & amode) == amode ? 0 : EACCES;
You are right, but such an optimization should be committed as
a the next step.
--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
[EMAIL PROTECTED] | TCP/IP since RFC 956
FreeBSD coreteam member | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Patch test & review: vop_stdaccess()
On Wed, Aug 16, 2000, Poul-Henning Kamp wrote: > > Please test and review this patch: > > http://phk.freebsd.dk/patch/vop_stdaccess.patch > > This patch creates a centralized function "vop_stdaccess()" which > does the "canonical" permission check on a vnode. > > This code was duplicated in 5 filesystems and morphed in a 6th. > > Files touched: > > Index: fs/hpfs/hpfs_vnops.c > Index: isofs/cd9660/cd9660_vnops.c > Index: kern/vfs_default.c > Index: miscfs/kernfs/kernfs_vnops.c > Index: msdosfs/msdosfs_vnops.c > Index: ntfs/ntfs_vnops.c > Index: sys/vnode.h > Index: ufs/ufs/ufs_vnops.c > > Linecount: removes: 243 adds: 79 net change: -164 I reviewed this last night and it looks good. Perhaps someone with credential clue could suggest a change to the cred_uid == 0 check .. ? Adrian -- Adrian ChaddNow 17-year-olds can't play a _video game_ <[EMAIL PROTECTED]>because its called violent - and real violence is still called dinner. -- [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Patch test & review: vop_stdaccess()
On Wed, 16 Aug 2000, Poul-Henning Kamp wrote:
> Please test and review this patch:
>
> http://phk.freebsd.dk/patch/vop_stdaccess.patch
Looks fine to me except vop_stdaccess() itself. Since VREAD,
VWRITE and VEXEC bits are carefully layed this function can be rewritten
to use single shift operation instead of 3 'or's:
if (cred->cr_uid != uid) {
amode >>= 3;
if (!groupmember(gid, cred))
amode >>= 3;
}
return (fmode & amode) == amode ? 0 : EACCES;
--
Boris Popov
http://www.butya.kz/~bp/
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Patch test & review: vop_stdaccess()
* Poul-Henning Kamp <[EMAIL PROTECTED]> [000816 14:52] wrote: > > Please test and review this patch: > > http://phk.freebsd.dk/patch/vop_stdaccess.patch Not tested, but looks good, I'd like to see it applied. -Alfred To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Patch test & review: vop_stdaccess()
Please test and review this patch: http://phk.freebsd.dk/patch/vop_stdaccess.patch This patch creates a centralized function "vop_stdaccess()" which does the "canonical" permission check on a vnode. This code was duplicated in 5 filesystems and morphed in a 6th. Files touched: Index: fs/hpfs/hpfs_vnops.c Index: isofs/cd9660/cd9660_vnops.c Index: kern/vfs_default.c Index: miscfs/kernfs/kernfs_vnops.c Index: msdosfs/msdosfs_vnops.c Index: ntfs/ntfs_vnops.c Index: sys/vnode.h Index: ufs/ufs/ufs_vnops.c Linecount: removes: 243 adds: 79 net change: -164 -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD coreteam member | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
