Re: accounting to appen only file

> I have been running with the equivalent patch on stable
> for a while and it works as supposed for me. However, since
> I'm not that familiar with the vn code I would like to get
> feedback if this might be harmfull in any way.

> Index: src/sys/kern//kern_acct.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/kern/kern_acct.c,v
> retrieving revision 1.44
> diff -u -r1.44 kern_acct.c
> --- src/sys/kern//kern_acct.c	16 May 2002 21:28:11 -0000	1.44
> +++ src/sys/kern//kern_acct.c	30 Jun 2002 19:39:55 -0000
> @@ -119,6 +119,9 @@
>  	struct nameidata nd;
>  	int error, flags;
>
> +	/* Open file for append writing only */
> +	flags = FWRITE | FAPPEND;
> +

This has some style bugs: excessive commenting, an unterminated sentence,
and some less than useful code restructuring (moving the initialization
of `flags'.  (This file is excessively commented in several places.  I
think the comments in acct_process() are just to show that it was
implemented in a clean room starting from documentation.)

>  	/* Make sure that the caller is root. */
>  	error = suser(td);

Excessive commenting in old code.

>  	if (error)
> @@ -126,20 +129,19 @@
>
>  	mtx_lock(&Giant);
>  	/*
> -	 * If accounting is to be started to a file, open that file for
> -	 * writing and make sure it's a 'normal'.
> +	 * If accounting is to be started to a file, open that file
> +	 * and make sure it's a 'normal'.
>  	 */
>  	if (SCARG(uap, path) != NULL) {
>  		NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_USERSPACE, SCARG(uap, path),
>  		       td);
> -		flags = FWRITE;

I would just change FWRITE globally to FWRITE | FAPPEND (and adjust
the comment to say "appending").  That would be a little chummy with
the implementation, but so is the existing and new code, and fixing
this is beyond the scope of the PR.

>  		error = vn_open(&nd, &flags, 0);

Note that the flags variable was only used to discard some changes made
by vn_open().  At least in the direct descendant of vn_open(), these changes
are limited to clearing O_CREAT and O_TRUNC, but we really shouldn't assume
this.  Strictly, we should record returned flags for use late like the `file'
layer would do.

>  		if (error)
>  			goto done2;
>  		NDFREE(&nd, NDF_ONLY_PNBUF);
>  		VOP_UNLOCK(nd.ni_vp, 0, td);
>  		if (nd.ni_vp->v_type != VREG) {
> -			vn_close(nd.ni_vp, FWRITE, td->td_ucred, td);
> +			vn_close(nd.ni_vp, flags, td->td_ucred, td);

This fixes an error case to use the returned flags.

>  			error = EACCES;
>  			goto done2;
>  		}
> @@ -151,7 +153,7 @@
>  	 */
>  	if (acctp != NULLVP || savacctp != NULLVP) {
>  		callout_stop(&acctwatch_callout);
> -		error = vn_close((acctp != NULLVP ? acctp : savacctp), FWRITE,
> +		error = vn_close((acctp != NULLVP ? acctp : savacctp), flags,

Here using the returned flags is (logically) worse than using hard-coded
flags, since we are closing a different file so we should strictly use the
flags that were returned when we opened that file, nit the flags for the
file that we just opened.

>  		    td->td_ucred, td);
>  		acctp = savacctp = NULLVP;
>  	}

I think FAPPEND should be spelled O_APPEND in the above.  The former
spelling is currently not used anywhere in *.c under /sys except in
alpha/osf1.  It should be only compatibility cruft for userland.

I doubt that vn_close() actually cares about the FAPPEND flag.

There are some more FWRITE's in acctwatch().  These are only for closes,
so the correctness of the above patches depends on vn_close() not caring.

Bruce


To Unsubscribe: send mail to [email protected]
with "unsubscribe freebsd-audit" in the body of the message