Re: [PATCH 1/2] kernel/acct.c: saner struct file treatment

[Al Viro <viro@zeniv.linux.org.uk>]

On Thu, Sep 25, 2025 at 07:56:30PM +0100, Al Viro wrote:
> On Thu, Sep 25, 2025 at 02:28:16PM +0200, Jan Kara wrote:
> 
> > This is mostly harmless (just the returned error code changed) but it is a
> > side effect of one thing I'd like to discuss: The original code uses
> > file_open_name(O_WRONLY|O_APPEND|O_LARGEFILE) to open the file to write
> > data to. The new code uses dentry_open() for that. Now one important
> > difference between these two is that dentry_open() doesn't end up calling
> > may_open() on the path and hence now acct_on() fails to check file
> > permissions which looks like a bug? Am I missing something Al?
> 
> You are not; a bug it is.  FWIW, I suspect that the right approach would
> be to keep file_open_name(), do all checks on result of that, then use
> 	mnt = mnt_clone_internal(&original_file->f_path);
> and from that point on same as now - opening the file to be used with
> dentry_open(), etc.  Original file would get dropped in the end of acct_on().
> I'll put together something along those lines and post it.

Something like this for incremental (completely untested at that point):

diff --git a/kernel/acct.c b/kernel/acct.c
index 30ae403ee322..61630110e29d 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -218,19 +218,21 @@ static int acct_on(const char __user *name)
 	/* Difference from BSD - they don't do O_APPEND */
 	const int open_flags = O_WRONLY|O_APPEND|O_LARGEFILE;
 	struct pid_namespace *ns = task_active_pid_ns(current);
-	struct path path __free(path_put) = {};		// in that order
+	struct filename *pathname __free(putname) = getname(name);
+	struct file *original_file __free(fput) = NULL;	// in that order
 	struct path internal __free(path_put) = {};	// in that order
 	struct file *file __free(fput_sync) = NULL;	// in that order
 	struct bsd_acct_struct *acct;
 	struct vfsmount *mnt;
 	struct fs_pin *old;
-	int err;
 
-	err = user_path_at(AT_FDCWD, name, LOOKUP_FOLLOW, &path);
-	if (err)
-		return err;
+	if (IS_ERR(pathname))
+		return PTR_ERR(pathname);
+	original_file = file_open_name(pathname, open_flags, 0);
+	if (IS_ERR(original_file))
+		return PTR_ERR(original_file);
 
-	mnt = mnt_clone_internal(&path);
+	mnt = mnt_clone_internal(&original_file->f_path);
 	if (IS_ERR(mnt))
 		return PTR_ERR(mnt);
 
@@ -268,7 +270,7 @@ static int acct_on(const char __user *name)
 	INIT_WORK(&acct->work, close_work);
 	init_completion(&acct->done);
 	mutex_lock_nested(&acct->lock, 1);	/* nobody has seen it yet */
-	pin_insert(&acct->pin, path.mnt);
+	pin_insert(&acct->pin, original_file->f_path.mnt);
 
 	rcu_read_lock();
 	old = xchg(&ns->bacct, &acct->pin);