[Devel] Re: [RFC][PATCH 2/3] c/r: [pty 1/2] allow allocation of desired pty slave

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl at librato.com):
>> During restart, we need to allocate pty slaves with the same
>> identifiers as recorded during checkpoint. Modify the allocation code
>> to allow an in-kernel caller to request a specific slave identifier.
>>
>> For this, add a new field to task_struct - 'required_id'. It will
>> hold the desired identifier when restoring a (master) pty.
> 
> This is really unfortunate.

:(

> 
> I assume you left the rather generic name 'required_id' because
> you intend to use it for other calls as well?  Which is even
> more unfortunate...

:'(

> 
> I know I'm generally the one who pushes for re-using existing
> helpers as much as possible, both to avoid missing existing
> security checks, and to prevent future code changes which forget
> to update the c/r code.
> 
> But in this case I don't think re-using fopen to ask the kernel
> to create a new device with more info than fopen usually gets is
> right.  Unfortunately, actually coming up with 'the right way' is
> escaping me at the moment :)
> 
> Though... then again...  your code is hardcoded for devpts and
> opening /dev/ptmx anyway, so surely we can create a new helper
> which takes a devpts_ns and index and returns the new devpts
> tty_struct without using the current->required_id?

The problem is that the way pty works: the index allocation takes
place inside ptmx_open(). As it is, I can't find of a way to
directly transfer a piece of data to it, because all we do is
call filp_open().

For this, we need add new functionality for pty code to

a) Allocate a master PTY without the corresponding file; this
function will be called from ptmx_open() and from restart code,
and will accept the "desired index" variable.

b) Attach an existing (master) PTY to a file pointer. This
will also be called from ptmx_open() and from restart code.

and

c) For restart only, manually do the work of filp_open() to
create a file, dentry, inode etc. This is what I really wanted
to avoid.

> 
> Then, if we ever want to support a tty type other than unix98,
> maybe we can more generally split up more like:

Other tty types have index that is indicated by the device minor
number (e.g. old style PTYs) or no index considerations :)

Oren.

> 
> 	1. add 'struct tty_struct *make_tty(struct tty_driver *driver,
> 			void *data) to tty_operations.  It is
> 			undefined for all but devpts.
> 	2. for devpts, the void *data includes an int devpts_ns_id
> 			and index
> 	3. the container-level checkpoint stores a relation between
> 		pathnames and devpts_ns_id.  devpts_ns_id is valid
> 		only for one checkpoint image.  The pathname is
> 		where that devpts_ns is mounted.  If we ever start
> 		checkpointing and restoring fs mounts from kernel,
> 		then we can tweak this treatment.  For now it is
> 		assumed (and verified) that at restart, pathname is
> 		in fact where a devpts instance is mounted.
> 	4. the tty_struct itself is checkpointed and restored.  It
> 		is restored using make_tty() in tty_operations,
> 		and for devpts the void *data  is a struct storing
> 		the devpts_ns_id and index.
> 	5. when a struct file for a devpts is restored, the tty_struct
> 		will already have been re-created.  The  tty_struct's objref
> 		is stored in the file struct checkpoint data.
> 
> It requires more finagling of the file/inode/tty_struct relationship
> though...  But I don't see required_id being acceptable upstream.
> 
> Of course I've been wrong about such things before.
> 
> -serge
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers