Re: [PATCH tty-next] devpts: Make each mount of devpts an independent filesystem.

[Eric W. Biederman]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Thu, Jun 2, 2016 at 8:29 AM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> The /dev/ptmx device node is changed to lookup the directory entry
>> "pts" in the same directory as the /dev/ptmx device node was opened
>> in.  If there is a "pts" entry and that entry is a devpts filesystem
>> /dev/ptmx uses that filesystem.  Otherwise the open of /dev/ptmx
>> fails.
>>
>> The DEVPTS_MULTIPLE_INSTANCES configuration option is removed,
>> so that userspace can now safely depend on each mount of devpts
>> creating a new instance of the filesystem.
>>
>> Each mount of devpts is now a separate and equal filesystem.
>
> Ok, this came outside the merge window, but the patches predate it by
> a lot, so I'm actually inclined to finally get this all sorted out and
> just apply it.
>
> Al, I'm not seeing anything horribly questionable in the path_pts()
> function, although I think it should just do lookup_one_len_unlocked()
> and follow_mount() instead of open-coding that hashing etc. So I
> actually prefer the ptmx_to_pts() function I posted back in April:
>

The problem with lookup_one_len_unlocked is that it still calls
inode_permission.

As per previous discussions we don't want the path based permission
checks involved in that lookup.

I also have a path_connected check after the dget_parent to ensure
there is not a crazy situation involved where the parent directory of
the ptmx dentry is not connected to it's mount.

Plus my version by not having the lookup_slow fallback that is in
lookup_one_len_unlocked never has to even consider the inode mutex or
any of that, so it is fundamentally simpler to deal with.  And of course
we are dealing with something that must be a mount point which pins the
dentry, which makes a lookup_slow fallback pointless.


>   static struct dentry *ptmx_to_pts(struct path *ptmx)
>   {
>       struct dentry *dev = dget_parent(ptmx->dentry);
>
>       if (dev) {
>           struct path path;
>
>           path.dentry = lookup_one_len_unlocked("pts", dev, 3);
>           if (path.dentry) {
               ^^^^^^^^^^^^^^^  That check is buggy because it needs at
               least an IS_ERR(path.dentry)
>               path.mnt = mntget(ptmx->mnt);
>               follow_mount(&path);
>               mntput(path.mnt);
>               return path.dentry;
>           }
>       }
>       return NULL;
>   }
>
> but I don't care *too* much. Al?
>
> Comments?
>
>                Linus

Eric