Ptyfs correction.

[Ilia Zykov]

Hello!
This patch corrects some wrong comments and
little memory leaks inside "ptyfs" and "ptm" driver.
Can be applied on "netbsd-7" and "current".
Thank you.
Ilia.

fs/ptyfs/ptyfs_subr.c | 47
++++++++++++++++++++++++++++++++++++-----------
fs/ptyfs/ptyfs_vfsops.c | 1 +
kern/tty_ptm.c | 18 ++----------------

3 files changed, 39 insertions(+), 27 deletions(-)

[Ilia Zykov]

Sorry, forgot free().
Ilia.

[Christos Zoulas]

-=-=-=-=-=-
Sorry, forgot free().

I think I would prefer to has a lock argument in the lookup function
and lock for the first call (the plain lookup) and not lock in the
second one (the lookup to insert). This feels safer to me and I don't
have to explain all the intricacies why locking is not needed which
just makes the code more fragile.

christos

[Ilia Zykov]

Post by Christos Zoulas

-=-=-=-=-=-
Sorry, forgot free().

I think I would prefer to has a lock argument in the lookup function
and lock for the first call (the plain lookup) and not lock in the
second one (the lookup to insert). This feels safer to me and I don't
have to explain all the intricacies why locking is not needed which
just makes the code more fragile.
christos

Hello!
- corrects some wrong comments
- add XXX warning
- increase security
- make pty_vn_open() private to tty_ptm.c

Thank you.
Ilia.

fs/ptyfs/ptyfs_subr.c | 6 ++++++
fs/ptyfs/ptyfs_vfsops.c | 6 ++++--
kern/tty_ptm.c | 17 +----------------
sys/pty.h | 3 +--
4 files changed, 12 insertions(+), 20 deletions(-)

[Christos Zoulas]

On Oct 15, 1:14pm, ***@izyk.ru (Ilia Zykov) wrote:
-- Subject: Re: Ptyfs correction.

| Hello!
| - corrects some wrong comments
| - add XXX warning
| - increase security
| - make pty_vn_open() private to tty_ptm.c
|
| Thank you.
| Ilia.

Can you please explain this:

- if (type == PTYFSptc)
+ /* Activate node only after we have grabbed device. */
+ if (type == PTYFSpts)


Thanks.
christos

[Ilia Zykov]

Post by Christos Zoulas
-- Subject: Re: Ptyfs correction.
| Hello!
| - corrects some wrong comments
| - add XXX warning
| - increase security
| - make pty_vn_open() private to tty_ptm.c
|
| Thank you.
| Ilia.
- if (type == PTYFSptc)
+ /* Activate node only after we have grabbed device. */
+ if (type == PTYFSpts)
Thanks.
christos

On SMP systems(with ptyfs multiple mounts) in this point (type ==
PTYFSptc) can be multiple threads.
Afterwards "ptm" driver leave only one(all other will grab other
devices), but will be time before ptyfs_inactive(),
when its will be have active this node for other mount points.
After, when "ptm" has grabbed device for this thread, it calls this
function only for one winner thread
with type == PTYFSpts. It's situation very difficult usage because we
do VOP_REVOKE(), but theoretically
possible. And more reliable activate device only at one mount point when
we have grabbed device.

Ilia.

[Christos Zoulas]

On Oct 15, 5:49pm, ***@izyk.ru (Ilia Zykov) wrote:
-- Subject: Re: Ptyfs correction.

|
| On 15.10.2014 16:55, Christos Zoulas wrote:
| > On Oct 15, 1:14pm, ***@izyk.ru (Ilia Zykov) wrote:
| > -- Subject: Re: Ptyfs correction.
| >
| > | Hello!
| > | - corrects some wrong comments
| > | - add XXX warning
| > | - increase security
| > | - make pty_vn_open() private to tty_ptm.c
| > |
| > | Thank you.
| > | Ilia.
| >
| > Can you please explain this:
| >
| > - if (type == PTYFSptc)
| > + /* Activate node only after we have grabbed device. */
| > + if (type == PTYFSpts)
| >
| >
| > Thanks.
| > christos
| >
|
| On SMP systems(with ptyfs multiple mounts) in this point (type ==
| PTYFSptc) can be multiple threads.
| Afterwards "ptm" driver leave only one(all other will grab other
| devices), but will be time before ptyfs_inactive(),
| when its will be have active this node for other mount points.
| After, when "ptm" has grabbed device for this thread, it calls this
| function only for one winner thread
| with type == PTYFSpts. It's situation very difficult usage because we
| do VOP_REVOKE(), but theoretically
| possible. And more reliable activate device only at one mount point when
| we have grabbed device.

Thanks, makes sense.

christos

[Ilia Zykov]

Post by Christos Zoulas
-- Subject: Re: Ptyfs correction.
|
| > -- Subject: Re: Ptyfs correction.
| >
| > | Hello!
| > | - corrects some wrong comments
| > | - add XXX warning
| > | - increase security
| > | - make pty_vn_open() private to tty_ptm.c
| > |
| > | Thank you.
| > | Ilia.
| >
| >
| > - if (type == PTYFSptc)
| > + /* Activate node only after we have grabbed device. */
| > + if (type == PTYFSpts)
| >
| >
| > Thanks.
| > christos
| >
|
| On SMP systems(with ptyfs multiple mounts) in this point (type ==
| PTYFSptc) can be multiple threads.
| Afterwards "ptm" driver leave only one(all other will grab other
| devices), but will be time before ptyfs_inactive(),
| when its will be have active this node for other mount points.
| After, when "ptm" has grabbed device for this thread, it calls this
| function only for one winner thread
| with type == PTYFSpts. It's situation very difficult usage because we
| do VOP_REVOKE(), but theoretically
| possible. And more reliable activate device only at one mount point when
| we have grabbed device.
Thanks, makes sense.
christos

Please.
Try this experiment, you need two terminals one for ordinary user, other for root.
1. In ordinary terminal run ptmxopen(output for sample):
./ptmxopen
Pty device is : /dev/ptmx
Name is : /dev/pts/3
Opened : /dev/pts/3

correct ptsopen.c for /dev/pts/3
run
./ptsopen
Pty device is : /dev/pts/3

2. In root terminal run
./ptmxopen.
Pty device is : /dev/ptmx
Name is : /dev/pts/3
Opened : /dev/pts/3

3. In ordinary terminal I have this output:
Got it. Device name is : /dev/pts/3

It's happened only with /dev/ptmx device.
I don't know why it isn't happened with /dev/ptm, maybe it have some
kernel lock between operations.

This patch can help for this too:
- if (type == PTYFSptc)
+ /* Activate node only after we have grabbed device. */
+ if (type == PTYFSpts)


Ilia.