linux-kernel - Re: [PATCH 0/2] rcu_dereference_check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140111063438.GY10038@linux.vnet.ibm.com>
Date:	Fri, 10 Jan 2014 22:34:38 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...IV.linux.org.uk>,
	Dipankar Sarma <dipankar@...ibm.com>,
	Eric Dumazet <edumazet@...gle.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

On Fri, Jan 10, 2014 at 04:34:59PM +0100, Oleg Nesterov wrote:
> On 01/08, Paul E. McKenney wrote:
> >
> > On Wed, Jan 08, 2014 at 04:19:18PM +0100, Oleg Nesterov wrote:
> > > On 01/08, Paul E. McKenney wrote:
> > > >
> > > > Another approach would be to add an argument to files_fdtable()
> > > > that is zero normally and one for "we know we don't need RCU
> > > > protection."  Then rcu_dereference_check() could be something
> > > > like the following:
> > > >
> > > > #define files_fdtable(files, c) \
> > > > 		(rcu_dereference_check_fdtable((files), (files)->fdt) || c)
> > > >
> > > > Would that work?
> > >
> > > Yes, I considered this optiion, but this needs much more uglifications^W
> > > changes.
> > >
> > > Either we need to change all users of files_fdtable(), or we need something
> > > like
> >
> > There are only about 20 uses of files_fdtable() in 3.12, with almost all
> > of them in fs/file.c.  So is changing all the users really all that
> > problematic?
> 
> But only one user, close_files(), needs files_fdtable(files, true). Why
> complicate the patch and the code? I think it would be better to simply
> change close_files() to use rcu_dereference_raw().
> 
> And note that rcu_dereference_check_fdtable() needs the new argument too.
> 
> And we should also take care of fcheck_files(),
> 
> > > 	static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
> > > 	{
> > > 		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
> > > 		struct file *file = NULL;
> > >
> > > 		if (fd < fdt->max_fds)
> > > 			file = rcu_dereference_raw(fdt->fd[fd]);
> > >
> > > 		return file;
> > > 	}
> > >
> > > 	static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
> > > 	{
> > > 		rcu_lockdep_assert(rcu_read_lock_held() ||
> > > 				   lockdep_is_held(files->file_lock),
> > > 				   "message");
> > > 		return __fcheck_files(files, fd);
> > > 	}
> 
> doesn't this look much simpler than adding the "bool unshared" argument
> and changing the callers?

I might be being too paranoid, but my concern with using rcu_lock_acquire()
and rcu_lock_release() is the possibility of code needing rcu_read_lock()
appearing somewhere in the function-call graph between rcu_lock_acquire()
and rcu_lock_release().  In that case, lockdep would be happy, but the
required RCU protection would not be present.

Sort of like my experience with people using RCU from idle.

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/