On Fri, Jan 18, 2019 at 10:55:52PM +0000, Al Viro wrote:
> On Fri, Jan 18, 2019 at 03:53:41PM +0100, Christian Brauner wrote:
> > We don't allow to unlink it since it is crucial for binderfs to be useable
> > but if we allow to rename it we make the unlink trivial to bypass. So
> > prevent renaming too and simply treat the control dentry as immutable.
> >
> > Take the opportunity and turn the check for the control dentry into a
> > separate helper is_binderfs_control_device() since it's now used in two
> > places.
> > Additionally, replace the custom rename dance we did with call to
> > simple_rename().
>
> Umm...
>
> > +static inline bool is_binderfs_control_device(const struct inode *inode,
> > + const struct dentry *dentry)
> > +{
> > + return BINDERFS_I(inode)->control_dentry == dentry;
> > +}
>
> What do you need an inode for?
BINDERFS_I() is called in is_binderfs_device() which is called in
binder.c:
static int binder_open(struct inode *nodp, struct file *filp)
{
<snip>
/* binderfs stashes devices in i_private */
if (is_binderfs_device(nodp))
binder_dev = nodp->i_private;
else
binder_dev = container_of(filp->private_data,
struct binder_device, miscdev);
<snip>
and it was just easier to have it take an inode as arg since that's what
binder_open() makes easily available. Just kept it this way for
is_binderfs_control_device() too but will switch the latter over now.
>
> static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> {
> return inode->i_sb->s_fs_info;
> }
>
> so it looks like all you care about is the superblock. Which can be
> had simply as dentry->d_sb...
Hm yes, I think I'll just do:
static inline bool is_binderfs_control_device(const struct dentry *dentry)
{
struct binderfs_info *info = dentry->d_sb->s_fs_info;
return info->control_dentry == dentry;
}
and pick up what you scratched below.
>
> Besides, what's the point of calling is_binderfs_device() in ->rename()?
> If your directory methods are given dentries from another filesystem,
> the kernel is already FUBAR. So your rename should simply do
> if (is_binderfs_control_device(old_dentry) ||
> is_binderfs_control_device(new_dentry))
> return -EPERM;
> return simple_rename(......);
> and that's it...
