On Thu, Aug 24, 2017 at 4:37 PM, Christian Brauner
wrote:
>
> In fact, /dev/ptmx being a symlink or bind-mount is the *standard* in
> containers
> even for non-user namespaced containers or containers that do not retain
> CAP_MKNOD.
Yes.
I think using /dev/pts/ptmx is nice from a kernel standpo
On Thu, Aug 24, 2017 at 06:01:36PM -0500, Eric W. Biederman wrote:
> Linus Torvalds writes:
>
> > On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
> > wrote:
> >>
> >> There are just enough weird one off scripts like xen image builder (I
> >> think that was the nasty test case that broke in de
On Thu, Aug 24, 2017 at 4:01 PM, Eric W. Biederman
wrote:
> Linus Torvalds writes:
>
>> On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
>> wrote:
>>>
>>> There are just enough weird one off scripts like xen image builder (I
>>> think that was the nasty test case that broke in debian) that I c
Linus Torvalds writes:
> On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
> wrote:
>>
>> There are just enough weird one off scripts like xen image builder (I
>> think that was the nasty test case that broke in debian) that I can't
>> imagine ever being able to responsibly remove the path base
On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
wrote:
>
> There are just enough weird one off scripts like xen image builder (I
> think that was the nasty test case that broke in debian) that I can't
> imagine ever being able to responsibly remove the path based lookups in
> /dev/ptmx. I do d
Linus Torvalds writes:
> On Thu, Aug 24, 2017 at 12:01 PM, Christian Brauner
> wrote:
>>
>> I've touched on this in my original message, I wonder whether we currently
>> support mounting devpts at a different a location and expect an open on a
>> newly created slave to work.
>
> Yes. That is
On Thu, Aug 24, 2017 at 1:24 PM, Stefan Lippers-Hollmann wrote:
>
> This patch seems to work, ssh, xterm (konsole5), real tty and pbuilder
> (creating- and updating the build chroots, just as well as building
> several fairly involved packages) are fine with this patch on top of
> v4.13-rc6-66-g14
Hi
[ sorry for the re-send, this accidentally only reached you, rather than
the mailing list and the other recipients as well ]
On 2017-08-24, Linus Torvalds wrote:
> On Thu, Aug 24, 2017 at 11:31 AM, Linus Torvalds
> wrote:
> >
> > It should just do a "return ptm_open_peer(file, tty, (int)arg)
Christian Brauner writes:
> On Aug 24, 2017 20:41, "Eric W. Biederman" wrote:
>
> > The implementation of TIOCGPTPEER has two issues.
> >
> > When /dev/ptmx (as opposed to
>
> I've touched on this in my original message, I wonder whether we
> currently support mounting devpts at a different a l
On Thu, Aug 24, 2017 at 12:22 PM, Linus Torvalds
wrote:
> [root@i7 dummy]# ../a.out
> I point to "/home/torvalds/dummy/pts/0"
Note: that "a.out" binary is modified from your original code.
It's modified to correctly print out the readdir() results, but it's
also modified to open just "ptmx"
Linus Torvalds writes:
> On Thu, Aug 24, 2017 at 11:40 AM, Eric W. Biederman
> wrote:
>>
>> Here is my tested version of the patch.
>
> Can you please take my cleanups to devpts_ptmx_path() too?
Let met take a look.
> Those 'goto err' statements are disgusting, when a plain 'return
> -ERRNO'
On Thu, Aug 24, 2017 at 12:01 PM, Christian Brauner
wrote:
>
> I've touched on this in my original message, I wonder whether we currently
> support mounting devpts at a different a location and expect an open on a
> newly created slave to work.
Yes. That is very much intended to work.
> Say I
On Thu, Aug 24, 2017 at 11:40 AM, Eric W. Biederman
wrote:
>
> Here is my tested version of the patch.
Can you please take my cleanups to devpts_ptmx_path() too?
Those 'goto err' statements are disgusting, when a plain 'return
-ERRNO' works cleaner.
And that "struct file *filp = NULL;" is bogus
Linus Torvalds writes:
> On Thu, Aug 24, 2017 at 11:06 AM, Linus Torvalds
> wrote:
>>
>> But that still fails the TIOCGPTPEER ioctl with an NULL pointer
>> dereference in devpts_mnt, I probably messed up when I fixed the
>> dentry refcount leak.
>
> No, that was another bug in the original patc
On Thu, Aug 24, 2017 at 11:31 AM, Linus Torvalds
wrote:
>
> It should just do a "return ptm_open_peer(file, tty, (int)arg);" instead.
Here's the actual tested patch. It "WorksForMe(tm)", including the
TIOCGPTPEER ioctl, and also verified that it gets the pathname right
in /proc, which was the ori
On Thu, Aug 24, 2017 at 11:13 AM, Linus Torvalds
wrote:
>
> The attached patch should work. It's Eric's original patch with
> various cleanups and fixes.
No, one more bug in there: Eric did
+ case TIOCGPTPEER:
+retval = ptm_open_peer(file, tty, (int)arg);
+break;
to call that pt
On Thu, Aug 24, 2017 at 11:06 AM, Linus Torvalds
wrote:
>
> But that still fails the TIOCGPTPEER ioctl with an NULL pointer
> dereference in devpts_mnt, I probably messed up when I fixed the
> dentry refcount leak.
No, that was another bug in the original patch.
ptm_open_peer() passed in 'filp'
On Thu, Aug 24, 2017 at 10:52 AM, Linus Torvalds
wrote:
>
> I'll test the fix.
Yes, that was it, and things work with that fixed.
But that still fails the TIOCGPTPEER ioctl with an NULL pointer
dereference in devpts_mnt, I probably messed up when I fixed the
dentry refcount leak.
On Thu, Aug 24, 2017 at 8:54 AM, Eric W. Biederman
wrote:
>
> Weird. There is at least one leak inducing bug in there. So perhaps
> that is the cause. *Scratches my head* Are you also testing the new
> ioctl?
I can verify, and it's not the leak. I tried your patch with that leak
fix (and the
Stefan Lippers-Hollmann writes:
> Hi
>
> On 2017-08-23, Eric W. Biederman wrote:
>> Linus Torvalds writes:
>> > On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds
>> > wrote:
> [...]
>> This is so far untested (except for compiling) but I think this will
>> work.
>>
>> I factor out devpts_ptmx
Linus Torvalds writes:
> On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman
> wrote:
>> -static int pty_get_peer(struct tty_struct *tty, int flags)
>> +int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
>> {
>> int fd = -1;
>> struct file *filp = NULL;
>>
Hi
On 2017-08-23, Eric W. Biederman wrote:
> Linus Torvalds writes:
> > On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds
> > wrote:
[...]
> This is so far untested (except for compiling) but I think this will
> work.
>
> I factor out devpts_ptmx_path out of devpts_acquire so the code
> doesn'
On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman
wrote:
> -static int pty_get_peer(struct tty_struct *tty, int flags)
> +int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
> {
> int fd = -1;
> struct file *filp = NULL;
> int retval = -EINVAL;
> +
Linus Torvalds writes:
> On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds
> wrote:
>>
>> Argh. And it's *not* fairly straightforward, because the
>> tty_operations "ioctl()" function pointer only gets 'struct tty *'.
>>
>> So in the TIOCGPTPEER path, we don't actually have access to the file
>> p
On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds
wrote:
>
> Argh. And it's *not* fairly straightforward, because the
> tty_operations "ioctl()" function pointer only gets 'struct tty *'.
>
> So in the TIOCGPTPEER path, we don't actually have access to the file
> pointer of the fd we're doing the io
On Wed, Aug 23, 2017 at 6:32 PM, Linus Torvalds
wrote:
>
> It should all be _fairly_ straightforward, but it's definitely a
> rather bigger change than that "just fix the path" patch was.
Argh. And it's *not* fairly straightforward, because the
tty_operations "ioctl()" function pointer only gets
On Wed, Aug 23, 2017 at 6:25 PM, Eric W. Biederman
wrote:
>
> The new behavior is that when we open ptmx we cache a path the slave
> pty.
Yes. It's not strictly "new", though - we've done that for a while,
and if you used /dev/pts/ptmx you'd even have had the *right* path
for a while ;)
And this
Linus Torvalds writes:
> On Wed, Aug 23, 2017 at 5:24 PM, Stefan Lippers-Hollmann wrote:
>>
>> This patch[1] as part of 4.13-rc6 (up to, at least,
>> v4.13-rc6-45-g6470812e2226) introduces a regression for me when using
>> pbuilder 0.228.7[2] (a helper to build Debian packages in a chroot and
>>
On Wed, Aug 23, 2017 at 5:42 PM, Linus Torvalds
wrote:
>
> Let me try to think about alteratives. Clearly this is a regression
> and I need to fix it, I just need to figure out _how_.
Ok, sadly, I think it's unfixable with the current model.
We literally used to keep the wrong 'struct path' arou
On Wed, Aug 23, 2017 at 5:24 PM, Stefan Lippers-Hollmann wrote:
>
> This patch[1] as part of 4.13-rc6 (up to, at least,
> v4.13-rc6-45-g6470812e2226) introduces a regression for me when using
> pbuilder 0.228.7[2] (a helper to build Debian packages in a chroot and
> to create and update its chroot
Hi
On 2017-08-16, Linus Torvalds wrote:
> On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
> wrote:
[...]
> Maybe this attached patch is better anyway. It's smaller, because it
> keeps more closely to the old code, and just adds a mntput() in all
> the exit cases, and depends on the "path_get()
On Wed, Aug 23, 2017 at 10:31:53AM -0500, Eric W. Biederman wrote:
> Christian Brauner writes:
>
> > On Wed, Aug 16, 2017 at 11:45 PM, Linus Torvalds
> > wrote:
> >> On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
> >> wrote:
> And Christian, if you can beat on this, that would be good.
Christian Brauner writes:
> On Wed, Aug 16, 2017 at 11:45 PM, Linus Torvalds
> wrote:
>> On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
>> wrote:
And Christian, if you can beat on this, that would be good.
>>>
>>> Yes, I can pound on this nicely with liblxc. We have patch
>>> ( https:/
Linus Torvalds writes:
> On Wed, Aug 16, 2017 at 12:56 PM, Linus Torvalds
> wrote:
>>
>> So the fact that we _don't_ get the right pathname for the pts entry
>> here means that something got screwed up in setting filp->f_path to
>> the right thing. We have all the code in place that _tries_ to d
Linus Torvalds writes:
> On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
> wrote:
>>
>> *Blink* You are right I missed that.
>>
>> In which case I am concerned about failures that make it to err_release.
>> Unless I am missing something (again) failures that jump to err_release
>> won't call
On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
wrote:
>
> *Blink* You are right I missed that.
>
> In which case I am concerned about failures that make it to err_release.
> Unless I am missing something (again) failures that jump to err_release
> won't call mntput and will result in a mnt lea
Linus Torvalds writes:
> On Wed, Aug 16, 2017 at 3:46 PM, Eric W. Biederman
> wrote:
>>> tty->link->driver_data = pts_path;
>>>
>>> retval = ptm_driver->ops->open(tty, filp);
>> ^^^
>>
>> If this open fails the code jumps to err_put_path which falls
>> through into out_pu
On Wed, Aug 16, 2017 at 3:46 PM, Eric W. Biederman
wrote:
>> tty->link->driver_data = pts_path;
>>
>> retval = ptm_driver->ops->open(tty, filp);
> ^^^
>
> If this open fails the code jumps to err_put_path which falls
> through into out_put_fsi.
No it doesn't.
err_path_put
Linus Torvalds writes:
> On Wed, Aug 16, 2017 at 1:30 PM, Linus Torvalds
> wrote:
>>
>> I suspect the easiest fix is to just add a "mnt" argument to
>> devpts_acquire(), It shouldn't be too painful. Let me try.
>
> Ok, here's a *very* lightly tested patch. It might have new bugs, but
> it makes
On Wed, Aug 16, 2017 at 11:45 PM, Linus Torvalds
wrote:
> On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
> wrote:
>>> And Christian, if you can beat on this, that would be good.
>>
>> Yes, I can pound on this nicely with liblxc. We have patch
>> ( https://github.com/lxc/lxc/pull/1728 ) up for
On Wed, Aug 16, 2017 at 11:55 PM, Linus Torvalds
wrote:
> On Wed, Aug 16, 2017 at 2:45 PM, Linus Torvalds
> wrote:
>>
>> But it would be good to just test this in general too, and make sure I
>> didn't screw up some reference count or something. The patch *looks*
>> obviously correct, but ...
>
>
On Wed, Aug 16, 2017 at 2:45 PM, Linus Torvalds
wrote:
>
> But it would be good to just test this in general too, and make sure I
> didn't screw up some reference count or something. The patch *looks*
> obviously correct, but ...
Side note: I suspect it should be marked for stable, but it's going
On Wed, Aug 16, 2017 at 2:37 PM, Christian Brauner
wrote:
>> And Christian, if you can beat on this, that would be good.
>
> Yes, I can pound on this nicely with liblxc. We have patch
> ( https://github.com/lxc/lxc/pull/1728 ) up for review that
> allocates pty fds from private devpts mounts in di
On Wed, Aug 16, 2017 at 11:03 PM, Linus Torvalds
wrote:
> On Wed, Aug 16, 2017 at 1:30 PM, Linus Torvalds
> wrote:
>>
>> I suspect the easiest fix is to just add a "mnt" argument to
>> devpts_acquire(), It shouldn't be too painful. Let me try.
>
> Ok, here's a *very* lightly tested patch. It mig
On Wed, Aug 16, 2017 at 1:30 PM, Linus Torvalds
wrote:
>
> I suspect the easiest fix is to just add a "mnt" argument to
> devpts_acquire(), It shouldn't be too painful. Let me try.
Ok, here's a *very* lightly tested patch. It might have new bugs, but
it makes your test program DTRT.
Al, mind go
On Wed, Aug 16, 2017 at 1:19 PM, Linus Torvalds
wrote:
>
> Anyway, I know what's wrong, next step is to figure out what the fix is.
Grr, We actually look up the right mnt in devpts_acquire(), but we
don't save it. We only save the superblock pointer, because that's
traditionally what we used.
I
On Wed, Aug 16, 2017 at 12:56 PM, Linus Torvalds
wrote:
>
> So the fact that we _don't_ get the right pathname for the pts entry
> here means that something got screwed up in setting filp->f_path to
> the right thing. We have all the code in place that _tries_ to do it,
> but it clearly has a bug
On Wed, Aug 16, 2017 at 12:48 PM, Christian Brauner
wrote:
>
> I thought - and sorry if I'm completely wrong here - that the proc name came
> from the open(const char *pathname, ...) call.
No. It comes from the path associated with the file descriptor, and is
expanded from the dentry tree.
Which
On Wed, Aug 16, 2017 at 11:48:48AM -0700, Linus Torvalds wrote:
> On Wed, Aug 16, 2017 at 11:26 AM, Linus Torvalds
> wrote:
> >
> > Hardcoding "/dev/pts/%d" is something that user space can already do.
> > The kernel can and should do better.
>
> Put another way: there's no point in applying the
On Wed, Aug 16, 2017 at 11:26 AM, Linus Torvalds
wrote:
>
> Hardcoding "/dev/pts/%d" is something that user space can already do.
> The kernel can and should do better.
Put another way: there's no point in applying the patch as-is, since
existing glibc ptsname() does the same thing better and fas
On Wed, Aug 16, 2017 at 10:12 AM, Christian Brauner
wrote:
>
> Recently the kernel has implemented the TIOCGPTPEER ioctl() call which allows
> users to retrieve an fd for the slave side of a pty solely based on the
> corresponding fd for the master side. The ioctl()-based fd retrieval however
> ca
Hi,
Recently the kernel has implemented the TIOCGPTPEER ioctl() call which allows
users to retrieve an fd for the slave side of a pty solely based on the
corresponding fd for the master side. The ioctl()-based fd retrieval however
causes the "/proc//fd/" symlink to point to the wrong dentry.
Curre
52 matches
Mail list logo
