Re: vmd 4/5: replace openpty(4) with a local function
> To fully abstract /dev/ptm in libutil, the API below would have to be > extended to have another function to open /dev/ptm in libutil as well, > eg. (better names would be desired): > > fd = openptmfd() > pledge() > fdopenpty(fd, ...) > fdopenpty(fd, ...) > fdopenpty(fd, ...) > fdopenpty(fd, ...) I'm not certain about prefixing the name with "fd"; after all it isn't just some fd, it is a VERY specific fd which was opened earlier as a resource. It is "some resource". Maybe this "some resource" is a concept found on other systems, where it might not be a fd.
Re: vmd 4/5: replace openpty(4) with a local function
> I think putting these in libutil is a good idea. tmux could use > them. I'd like to have openptmfd() as you suggest as well - it'd be nice > to hide PATH_PTMDEV as well as the PTMGET. > > Life would be a lot easier for portable if there was fdforkpty() as > well. I agree. Are the -portable versions of such an interface trivial?
Re: vmd 4/5: replace openpty(4) with a local function
Hi
I think putting these in libutil is a good idea. tmux could use
them. I'd like to have openptmfd() as you suggest as well - it'd be nice
to hide PATH_PTMDEV as well as the PTMGET.
Life would be a lot easier for portable if there was fdforkpty() as
well.
On Mon, Feb 27, 2017 at 07:00:03PM +0100, Reyk Floeter wrote:
> On Mon, Feb 27, 2017 at 10:19:28AM -0700, Theo de Raadt wrote:
> > > On Mon, Feb 27, 2017 at 10:55:31AM +0100, Reyk Floeter wrote:
> > > > The following diff is not really needed without just yet, but:
> > > > - openening /dev/ptm in advance might allow better pledge in the future
> > > > - customizing "openpty" will allow to do what we need next
> > > > Since openpty(4) is libutil and not libc, it should be fine not using
> > > > it.
> > > >
> > > > OK?
> > > >
> > > > Replace openpty(3) with local function that uses pre-opened
> > > > /dev/ptm fd
> > > >
> > > > This allows more flexibility for upcoming changes and better pledge.
> > > > We also didn't use half of the features of libutil's openpty
> > > > function.
> > > > Additionally, make sure that the ttys are closed correctly on
> > > > shutdown.
> >
> > This is related to the change that happened in tmux, when PTM ioctl was
> > locked down in pledge ioctl. The primary purpose of the change, was that
> > review of the PTM ioctl codepaths showed it was a lot of risk to keep in
> > pledge "tty" programs, so it makes sense that some of them get restructured.
> >
> > That means the libutil functions aren't always used. We should consider
> > whether raw ioctl PTMGET in programs is "good" or "bad". vmd is openbsd
> > only, but tmux now has this in the openbsd-specific code.
> >
> > If this goes PTMGET approach goes any further, we should think about a
> > new interface in libutil which hides the ioctl in a differnet way, and
> > convert programs to it. Just a thought..
> >
>
> I had a diff for libutil but millert@ (or was it nicm@?) told me that
> it is better to keep it as a local change. I cannot remember and
> unfortunately I cannot find the mail where it was mentioned.
>
> I attached the old diff for libutil as a reference.
>
> To fully abstract /dev/ptm in libutil, the API below would have to be
> extended to have another function to open /dev/ptm in libutil as well,
> eg. (better names would be desired):
>
> fd = openptmfd()
> pledge()
> fdopenpty(fd, ...)
> fdopenpty(fd, ...)
> fdopenpty(fd, ...)
> fdopenpty(fd, ...)
>
> Reyk
>
> Index: lib/libutil/openpty.3
> ===
> RCS file: /cvs/src/lib/libutil/openpty.3,v
> retrieving revision 1.17
> diff -u -p -u -p -r1.17 openpty.3
> --- lib/libutil/openpty.3 28 Aug 2015 19:54:06 - 1.17
> +++ lib/libutil/openpty.3 25 Jan 2017 08:40:46 -
> @@ -44,6 +44,8 @@
> .Ft int
> .Fn openpty "int *amaster" "int *aslave" "char *name" "struct termios
> *termp" "struct winsize *winp"
> .Ft int
> +.Fn fdopenpty "int ptmfd" "int *amaster" "int *aslave" "char *name" "struct
> termios *termp" "struct winsize *winp"
> +.Ft int
> .Fn login_tty "int fd"
> .Ft pid_t
> .Fn forkpty "int *amaster" "char *name" "struct termios *termp" "struct
> winsize *winp"
> @@ -88,6 +90,15 @@ the caller, permissions are set to corre
> uses of that device are revoked (see
> .Xr revoke 2
> for details).
> +.Pp
> +The
> +.Fn fdopenpty
> +function works like
> +.Fn openpty
> +but expects a pre-opened
> +.Pa /dev/ptm
> +file descriptor
> +.Fa ptmfd .
> .Pp
> The
> .Fn login_tty
> Index: lib/libutil/pty.c
> ===
> RCS file: /cvs/src/lib/libutil/pty.c,v
> retrieving revision 1.20
> diff -u -p -u -p -r1.20 pty.c
> --- lib/libutil/pty.c 30 Aug 2016 14:44:45 - 1.20
> +++ lib/libutil/pty.c 25 Jan 2017 08:40:46 -
> @@ -57,11 +57,30 @@ openpty(int *amaster, int *aslave, char
> fd = open(PATH_PTMDEV, O_RDWR|O_CLOEXEC);
> if (fd == -1)
> return (-1);
> - if ((ioctl(fd, PTMGET, ) == -1)) {
> +
> + if (fdopenpty(fd, amaster, aslave, name, termp, winp) == -1) {
> close(fd);
> return (-1);
> }
> close(fd);
> +
> + return (0);
> +}
> +
> +int
> +fdopenpty(int ptmfd, int *amaster, int *aslave, char *name,
> +struct termios *termp, struct winsize *winp)
> +{
> + int master, slave;
> + struct ptmget ptm;
> +
> + /*
> + * Use /dev/ptm and the PTMGET ioctl to get a properly set up and
> + * owned pty/tty pair.
> + */
> + if ((ioctl(ptmfd, PTMGET, ) == -1))
> + return (-1);
> +
> master = ptm.cfd;
> slave = ptm.sfd;
> if (name) {
> Index: lib/libutil/util.h
> ===
> RCS file: /cvs/src/lib/libutil/util.h,v
> retrieving revision 1.34
> diff -u -p -u -p -r1.34 util.h
> --- lib/libutil/util.h3 Jun
Re: vmd 4/5: replace openpty(4) with a local function
On Mon, Feb 27, 2017 at 10:19:28AM -0700, Theo de Raadt wrote:
> > On Mon, Feb 27, 2017 at 10:55:31AM +0100, Reyk Floeter wrote:
> > > The following diff is not really needed without just yet, but:
> > > - openening /dev/ptm in advance might allow better pledge in the future
> > > - customizing "openpty" will allow to do what we need next
> > > Since openpty(4) is libutil and not libc, it should be fine not using it.
> > >
> > > OK?
> > >
> > > Replace openpty(3) with local function that uses pre-opened /dev/ptm
> > > fd
> > >
> > > This allows more flexibility for upcoming changes and better pledge.
> > > We also didn't use half of the features of libutil's openpty function.
> > > Additionally, make sure that the ttys are closed correctly on
> > > shutdown.
>
> This is related to the change that happened in tmux, when PTM ioctl was
> locked down in pledge ioctl. The primary purpose of the change, was that
> review of the PTM ioctl codepaths showed it was a lot of risk to keep in
> pledge "tty" programs, so it makes sense that some of them get restructured.
>
> That means the libutil functions aren't always used. We should consider
> whether raw ioctl PTMGET in programs is "good" or "bad". vmd is openbsd
> only, but tmux now has this in the openbsd-specific code.
>
> If this goes PTMGET approach goes any further, we should think about a
> new interface in libutil which hides the ioctl in a differnet way, and
> convert programs to it. Just a thought..
>
I had a diff for libutil but millert@ (or was it nicm@?) told me that
it is better to keep it as a local change. I cannot remember and
unfortunately I cannot find the mail where it was mentioned.
I attached the old diff for libutil as a reference.
To fully abstract /dev/ptm in libutil, the API below would have to be
extended to have another function to open /dev/ptm in libutil as well,
eg. (better names would be desired):
fd = openptmfd()
pledge()
fdopenpty(fd, ...)
fdopenpty(fd, ...)
fdopenpty(fd, ...)
fdopenpty(fd, ...)
Reyk
Index: lib/libutil/openpty.3
===
RCS file: /cvs/src/lib/libutil/openpty.3,v
retrieving revision 1.17
diff -u -p -u -p -r1.17 openpty.3
--- lib/libutil/openpty.3 28 Aug 2015 19:54:06 - 1.17
+++ lib/libutil/openpty.3 25 Jan 2017 08:40:46 -
@@ -44,6 +44,8 @@
.Ft int
.Fn openpty "int *amaster" "int *aslave" "char *name" "struct termios *termp"
"struct winsize *winp"
.Ft int
+.Fn fdopenpty "int ptmfd" "int *amaster" "int *aslave" "char *name" "struct
termios *termp" "struct winsize *winp"
+.Ft int
.Fn login_tty "int fd"
.Ft pid_t
.Fn forkpty "int *amaster" "char *name" "struct termios *termp" "struct
winsize *winp"
@@ -88,6 +90,15 @@ the caller, permissions are set to corre
uses of that device are revoked (see
.Xr revoke 2
for details).
+.Pp
+The
+.Fn fdopenpty
+function works like
+.Fn openpty
+but expects a pre-opened
+.Pa /dev/ptm
+file descriptor
+.Fa ptmfd .
.Pp
The
.Fn login_tty
Index: lib/libutil/pty.c
===
RCS file: /cvs/src/lib/libutil/pty.c,v
retrieving revision 1.20
diff -u -p -u -p -r1.20 pty.c
--- lib/libutil/pty.c 30 Aug 2016 14:44:45 - 1.20
+++ lib/libutil/pty.c 25 Jan 2017 08:40:46 -
@@ -57,11 +57,30 @@ openpty(int *amaster, int *aslave, char
fd = open(PATH_PTMDEV, O_RDWR|O_CLOEXEC);
if (fd == -1)
return (-1);
- if ((ioctl(fd, PTMGET, ) == -1)) {
+
+ if (fdopenpty(fd, amaster, aslave, name, termp, winp) == -1) {
close(fd);
return (-1);
}
close(fd);
+
+ return (0);
+}
+
+int
+fdopenpty(int ptmfd, int *amaster, int *aslave, char *name,
+struct termios *termp, struct winsize *winp)
+{
+ int master, slave;
+ struct ptmget ptm;
+
+ /*
+* Use /dev/ptm and the PTMGET ioctl to get a properly set up and
+* owned pty/tty pair.
+*/
+ if ((ioctl(ptmfd, PTMGET, ) == -1))
+ return (-1);
+
master = ptm.cfd;
slave = ptm.sfd;
if (name) {
Index: lib/libutil/util.h
===
RCS file: /cvs/src/lib/libutil/util.h,v
retrieving revision 1.34
diff -u -p -u -p -r1.34 util.h
--- lib/libutil/util.h 3 Jun 2013 21:07:02 - 1.34
+++ lib/libutil/util.h 25 Jan 2017 08:40:46 -
@@ -99,6 +99,7 @@ void pw_copy(int, int, const struct pass
intpw_scan(char *, struct passwd *, int *);
void pw_error(const char *, int, int);
intopenpty(int *, int *, char *, struct termios *, struct winsize *);
+intfdopenpty(int, int *, int *, char *, struct termios *, struct winsize
*);
intopendisk(const char *, int, char *, size_t, int);
pid_t forkpty(int *, char *, struct termios *, struct winsize *);
int
Re: vmd 4/5: replace openpty(4) with a local function
> On Mon, Feb 27, 2017 at 10:55:31AM +0100, Reyk Floeter wrote: > > The following diff is not really needed without just yet, but: > > - openening /dev/ptm in advance might allow better pledge in the future > > - customizing "openpty" will allow to do what we need next > > Since openpty(4) is libutil and not libc, it should be fine not using it. > > > > OK? > > > > Replace openpty(3) with local function that uses pre-opened /dev/ptm fd > > > > This allows more flexibility for upcoming changes and better pledge. > > We also didn't use half of the features of libutil's openpty function. > > Additionally, make sure that the ttys are closed correctly on shutdown. This is related to the change that happened in tmux, when PTM ioctl was locked down in pledge ioctl. The primary purpose of the change, was that review of the PTM ioctl codepaths showed it was a lot of risk to keep in pledge "tty" programs, so it makes sense that some of them get restructured. That means the libutil functions aren't always used. We should consider whether raw ioctl PTMGET in programs is "good" or "bad". vmd is openbsd only, but tmux now has this in the openbsd-specific code. If this goes PTMGET approach goes any further, we should think about a new interface in libutil which hides the ioctl in a differnet way, and convert programs to it. Just a thought..
Re: vmd 4/5: replace openpty(4) with a local function
On Mon, Feb 27, 2017 at 10:55:31AM +0100, Reyk Floeter wrote:
> The following diff is not really needed without just yet, but:
> - openening /dev/ptm in advance might allow better pledge in the future
> - customizing "openpty" will allow to do what we need next
> Since openpty(4) is libutil and not libc, it should be fine not using it.
>
> OK?
>
> Replace openpty(3) with local function that uses pre-opened /dev/ptm fd
>
> This allows more flexibility for upcoming changes and better pledge.
> We also didn't use half of the features of libutil's openpty function.
> Additionally, make sure that the ttys are closed correctly on shutdown.
>
ok gilles@
> diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
> index f35a3b3..a16c143 100644
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -126,10 +126,9 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid)
> struct vmop_create_params *vmc = >vm_params;
> struct vm_create_params *vcp = >vmc_params;
> unsigned int i;
> - int fd = -1, ttys_fd;
> + int fd = -1;
> int kernfd = -1, *diskfds = NULL, *tapfds = NULL;
> int saved_errno = 0;
> - char ptyname[VM_TTYNAME_MAX];
> char ifname[IF_NAMESIZE], *s;
> char path[PATH_MAX];
> unsigned int unit;
> @@ -241,12 +240,11 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
> uint32_t peerid)
>
> /* Open TTY */
> if (vm->vm_ttyname == NULL) {
> - if (openpty(>vm_tty, _fd, ptyname, NULL, NULL) == -1 ||
> - (vm->vm_ttyname = strdup(ptyname)) == NULL) {
> - log_warn("%s: can't open tty %s", __func__, ptyname);
> + if (vm_opentty(vm) == -1) {
> + log_warn("%s: can't open tty %s", __func__,
> + vm->vm_ttyname == NULL ? "" : vm->vm_ttyname);
> goto fail;
> }
> - close(ttys_fd);
> }
> if ((fd = dup(vm->vm_tty)) == -1) {
> log_warn("%s: can't re-open tty %s", __func__, vm->vm_ttyname);
> diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> index a01e40f..ed678ee 100644
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -20,6 +20,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include
> #include
> @@ -33,7 +35,6 @@
> #include
> #include
> #include
> -#include
>
> #include "proc.h"
> #include "vmd.h"
> @@ -484,6 +485,9 @@ vmd_configure(void)
> struct vmd_vm *vm;
> struct vmd_switch *vsw;
>
> + if ((env->vmd_ptmfd = open(PATH_PTMDEV, O_RDWR|O_CLOEXEC)) == -1)
> + fatal("open %s", PATH_PTMDEV);
> +
> /*
>* pledge in the parent process:
>* stdio - for malloc and basic I/O including events.
> @@ -605,6 +609,12 @@ vmd_reload(unsigned int reset, const char *filename)
> void
> vmd_shutdown(void)
> {
> + struct vmd_vm *vm, *vm_next;
> +
> + TAILQ_FOREACH_SAFE(vm, env->vmd_vms, vm_entry, vm_next) {
> + vm_remove(vm);
> + }
> +
> proc_kill(>vmd_ps);
> free(env);
>
> @@ -702,16 +712,8 @@ vm_stop(struct vmd_vm *vm, int keeptty)
> close(vm->vm_kernel);
> vm->vm_kernel = -1;
> }
> -
> - if (keeptty)
> - return;
> -
> - if (vm->vm_tty != -1) {
> - close(vm->vm_tty);
> - vm->vm_tty = -1;
> - }
> - free(vm->vm_ttyname);
> - vm->vm_ttyname = NULL;
> + if (!keeptty)
> + vm_closetty(vm);
> }
>
> void
> @@ -769,6 +771,7 @@ vm_register(struct privsep *ps, struct vmop_create_params
> *vmc,
>
> memcpy(>vm_params, vmc, sizeof(vm->vm_params));
> vm->vm_pid = -1;
> + vm->vm_tty = -1;
>
> for (i = 0; i < vcp->vcp_ndisks; i++)
> vm->vm_disks[i] = -1;
> @@ -787,12 +790,45 @@ vm_register(struct privsep *ps, struct
> vmop_create_params *vmc,
>
> *ret_vm = vm;
> return (0);
> -fail:
> + fail:
> if (errno == 0)
> errno = EINVAL;
> return (-1);
> }
>
> +int
> +vm_opentty(struct vmd_vm *vm)
> +{
> + struct ptmgetptm;
> +
> + /*
> + * Open tty with pre-opened PTM fd
> + */
> + if ((ioctl(env->vmd_ptmfd, PTMGET, ) == -1))
> + return (-1);
> +
> + vm->vm_tty = ptm.cfd;
> + close(ptm.sfd);
> + if ((vm->vm_ttyname = strdup(ptm.sn)) == NULL)
> + goto fail;
> +
> + return (0);
> + fail:
> + vm_closetty(vm);
> + return (-1);
> +}
> +
> +void
> +vm_closetty(struct vmd_vm *vm)
> +{
> + if (vm->vm_tty != -1) {
> + close(vm->vm_tty);
> + vm->vm_tty = -1;
> + }
> + free(vm->vm_ttyname);
> + vm->vm_ttyname = NULL;
> +}
> +
> void
>
vmd 4/5: replace openpty(4) with a local function
The following diff is not really needed without just yet, but:
- openening /dev/ptm in advance might allow better pledge in the future
- customizing "openpty" will allow to do what we need next
Since openpty(4) is libutil and not libc, it should be fine not using it.
OK?
Replace openpty(3) with local function that uses pre-opened /dev/ptm fd
This allows more flexibility for upcoming changes and better pledge.
We also didn't use half of the features of libutil's openpty function.
Additionally, make sure that the ttys are closed correctly on shutdown.
diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
index f35a3b3..a16c143 100644
--- usr.sbin/vmd/config.c
+++ usr.sbin/vmd/config.c
@@ -126,10 +126,9 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid)
struct vmop_create_params *vmc = >vm_params;
struct vm_create_params *vcp = >vmc_params;
unsigned int i;
- int fd = -1, ttys_fd;
+ int fd = -1;
int kernfd = -1, *diskfds = NULL, *tapfds = NULL;
int saved_errno = 0;
- char ptyname[VM_TTYNAME_MAX];
char ifname[IF_NAMESIZE], *s;
char path[PATH_MAX];
unsigned int unit;
@@ -241,12 +240,11 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm,
uint32_t peerid)
/* Open TTY */
if (vm->vm_ttyname == NULL) {
- if (openpty(>vm_tty, _fd, ptyname, NULL, NULL) == -1 ||
- (vm->vm_ttyname = strdup(ptyname)) == NULL) {
- log_warn("%s: can't open tty %s", __func__, ptyname);
+ if (vm_opentty(vm) == -1) {
+ log_warn("%s: can't open tty %s", __func__,
+ vm->vm_ttyname == NULL ? "" : vm->vm_ttyname);
goto fail;
}
- close(ttys_fd);
}
if ((fd = dup(vm->vm_tty)) == -1) {
log_warn("%s: can't re-open tty %s", __func__, vm->vm_ttyname);
diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
index a01e40f..ed678ee 100644
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -20,6 +20,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -33,7 +35,6 @@
#include
#include
#include
-#include
#include "proc.h"
#include "vmd.h"
@@ -484,6 +485,9 @@ vmd_configure(void)
struct vmd_vm *vm;
struct vmd_switch *vsw;
+ if ((env->vmd_ptmfd = open(PATH_PTMDEV, O_RDWR|O_CLOEXEC)) == -1)
+ fatal("open %s", PATH_PTMDEV);
+
/*
* pledge in the parent process:
* stdio - for malloc and basic I/O including events.
@@ -605,6 +609,12 @@ vmd_reload(unsigned int reset, const char *filename)
void
vmd_shutdown(void)
{
+ struct vmd_vm *vm, *vm_next;
+
+ TAILQ_FOREACH_SAFE(vm, env->vmd_vms, vm_entry, vm_next) {
+ vm_remove(vm);
+ }
+
proc_kill(>vmd_ps);
free(env);
@@ -702,16 +712,8 @@ vm_stop(struct vmd_vm *vm, int keeptty)
close(vm->vm_kernel);
vm->vm_kernel = -1;
}
-
- if (keeptty)
- return;
-
- if (vm->vm_tty != -1) {
- close(vm->vm_tty);
- vm->vm_tty = -1;
- }
- free(vm->vm_ttyname);
- vm->vm_ttyname = NULL;
+ if (!keeptty)
+ vm_closetty(vm);
}
void
@@ -769,6 +771,7 @@ vm_register(struct privsep *ps, struct vmop_create_params
*vmc,
memcpy(>vm_params, vmc, sizeof(vm->vm_params));
vm->vm_pid = -1;
+ vm->vm_tty = -1;
for (i = 0; i < vcp->vcp_ndisks; i++)
vm->vm_disks[i] = -1;
@@ -787,12 +790,45 @@ vm_register(struct privsep *ps, struct vmop_create_params
*vmc,
*ret_vm = vm;
return (0);
-fail:
+ fail:
if (errno == 0)
errno = EINVAL;
return (-1);
}
+int
+vm_opentty(struct vmd_vm *vm)
+{
+ struct ptmgetptm;
+
+ /*
+* Open tty with pre-opened PTM fd
+*/
+ if ((ioctl(env->vmd_ptmfd, PTMGET, ) == -1))
+ return (-1);
+
+ vm->vm_tty = ptm.cfd;
+ close(ptm.sfd);
+ if ((vm->vm_ttyname = strdup(ptm.sn)) == NULL)
+ goto fail;
+
+ return (0);
+ fail:
+ vm_closetty(vm);
+ return (-1);
+}
+
+void
+vm_closetty(struct vmd_vm *vm)
+{
+ if (vm->vm_tty != -1) {
+ close(vm->vm_tty);
+ vm->vm_tty = -1;
+ }
+ free(vm->vm_ttyname);
+ vm->vm_ttyname = NULL;
+}
+
void
switch_remove(struct vmd_switch *vsw)
{
diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
index e371112..26d345c 100644
--- usr.sbin/vmd/vmd.h
+++ usr.sbin/vmd/vmd.h
@@ -185,6 +185,7 @@ struct vmd {
struct switchlist *vmd_switches;
int
