Re: [PATCH] Open a new file instance if no read permissions on files

From: Mimi Zohar <zohar@linux.ibm.com>
To: Goldwyn Rodrigues <rgoldwyn@suse.de>
Cc: linux-integrity@vger.kernel.org, linux-unionfs@vger.kernel.org,
	iforster@suse.de, fvogt@suse.de, miklos@szeredi.hu
Subject: Re: [PATCH] Open a new file instance if no read permissions on files
Date: Mon, 08 Oct 2018 09:27:59 -0400	[thread overview]
Message-ID: <1539005279.15382.119.camel@linux.ibm.com> (raw)
In-Reply-To: <20181008121455.za53z7kgfizgtiv7@merlin>

On Mon, 2018-10-08 at 07:14 -0500, Goldwyn Rodrigues wrote:

> 
> > >  
> > > +	if (!(file->f_mode & FMODE_READ)) {
> > > +		struct file *f;
> > 
> > I would define "struct file *f = file" above, at the beginning of
> > function, and "free(f)" below, without modifying "file".
> 
> I suppose you mean fput(f).

yes

> Okay, if it makes code more understandable.

Thanks
> 
> > 
> > > +		int flags = file->f_flags & ~(O_WRONLY | O_APPEND | O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
> > 
> > Doesn't O_RDONLY need to be added?
> 
> No. O_RDONLY is zero. But I think I should add it for readability. The
> compiler will optimize it eventually.
> 
> > Please fix the line length.
> > 
> > 
> > > +		f = dentry_open(&file->f_path, flags, file->f_cred);
> > > +		if (IS_ERR(f))
> > > +			return PTR_ERR(f);

It's late in the release cycle to be making this change.  Would it
make sense for now to fallback to modifying the original file
descriptor on failure and emit a message?

Mimi

> > > +		read = 1;
> > > +		file = f;
> > 
> > With the above change, no need to modify "file".
> 