Re: Should rm(1) -Pf change file permission?

[Ingo Schwarze]

Hi Gregoire,

Gregoire Jadi wrote on Tue, Apr 17, 2018 at 11:44:41AM +0200:
> Ingo Schwarze  writes:

>> Feedback is welcome both on the general idea and on the specific
>> implementation.

The result of that feeback was "we don't want it, the whole
idea of changing this is pointless and dangerous", so i deleted
my patch quite some time ago.

The conversation resulted in a clarification in the manual page,
though.

> Thank you for working on this.  I have tested your patch
> and both `rm -P` and `rm -Pf` work as expected.

Sorry, it won't be committed, it was definitely rejected.
Besides, it wasn't correct, there were race conditions.
Working on improving it would be a waste of time.

Yours,
  Ingo

Re: Kernel size beyond 16 MB on amd64

[Franco Fichtner]

Hi Stuart et al.,

Sorry for the delay.  Meanwhile, I've been reproducing the
issue on 6.3 by adding device rd and increasing MINIROOTSIZE
to grow the non-gdb amd64 kernel beyond 16 MB.  The kernel
simply fails to boot.

> If the kernel should grow to a point where we run past some limit, we'll fix
> it. Right now, bsd.gdb kernels are > 50MB and load fine.

While it's all true that there are no problems with bsd.gdb,
the fact of the matter is also that even if such a bloated
kernel as mentioned in the first paragraph is stripped below
the 16 MB point it fails to boot.  It even fails to boot if
it is compressed down to a few MB, which means it has nothing
to do with the actual size, but rather how its internal objects
are addressed.

The kicker is that during a 5.8 -> 5.9 transition i386 stopped
behaving as bad as amd64 in this regard so that there is a i386
kernel that performs better than amd64 one is hardly growing as
fast as the other.

To make a long story short, the answer actually was in the
commit mentioned in the original message[1] as NKL2_KIMG_ENTRIES
is the limiting factor here killing kernel boot when embedded
total object size grows beyond a certain 16 MB point.  Changing
this to e.g. 32 allows kernels to boot as long as they in turn
are not too big to hit the next barrier.

> That commit was only to move the kernel phys load address to 16MB.

That's not true, Mike, and I assume you already knew it.  ;)

Feel free to ignore this or take it as a friendly reminder that
not everything is "write smaller code" or "[kernels] load fine".


Cheers,
Franco

[1] https://github.com/openbsd/src/commit/453010f2034

Re: FREF(9) in unp_internalize()

[Todd C. Miller]

On Tue, 17 Apr 2018 09:50:49 +0200, Martin Pieuchot wrote:

> Sure, here's an updated diff.  It also moves the FRELE(9) in the error
> loop down as suggested by visa@.

OK millert@

 - todd

Re: Should rm(1) -Pf change file permission?

[Grégoire Jadi]

Ingo Schwarze  writes:

Hi,

> Hi,
>
> Gregoire Jadi wrote on Fri, Mar 30, 2018 at 06:07:42PM +0200:
>
>> While working on a port of keyringer, I observed the following behavior
>> of rm(1) with the -P option: if the file does not have write permission,
>> the file is removed without being overwritten.
>> 
>> This is not the same behavior as shred(1) (from sysutils/coreutils) which
>> do not remove the file if it cannot be overwritten. If the -f option is
>> used with shred(1), the permission of the file are changed to allow
>> writing if necessary.
>> 
>> Is the current behavior desired? (need to update the manpage?)
>> 
>> Or should `rm -P` and `rm -Pf` have the same behavior as shred. That is:
>> `rm -P` should fail without removing the file if the file cannot be
>> overwritten and `rm -Pf` should change the permission and overwrite
>> file.
>
> That makes more sense to me than the current behaviour.
>
> In addition, when running with -P and without -f and the user
> interactive confirms that the missing write protection shall be
> overriden, changing the permissions seems right to me, too.
>
> Here is a patch implementing that.
>
> Lightly tested by hand by running various combinations of rm(1)
>  * with and without -P
>  * with and without -f
>  * on files with different permissions
>
> Feedback is welcome both on the general idea and on the specific
> implementation.

Thank you for working on this. I have tested your patch and both `rm -P`
and `rm -Pf` work as expected.

I have no opinion on your implementation but it does solve the problem
and make rm(1) more consistent with gshred(1).

Best,

> Note that the return value of rm_overwrite() is currently unused.
> So we are free to change it to reflect success or failure, such
> that deleting the file can be skipped in case of failure even
> after trying to relax the permissions.  Also note that checking
> fflag in rw_overwrite is not needed (and would even be wrong)
> because the program doesn't get there in the first place unless
> fflag was given or deletion was interactively confirmed.
>
> Yours,
>   Ingo
>
> Index: rm.c
> ===
> RCS file: /cvs/src/bin/rm/rm.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 rm.c
> --- rm.c  27 Jun 2017 21:49:47 -  1.42
> +++ rm.c  31 Mar 2018 17:02:37 -
> @@ -103,7 +103,7 @@ main(int argc, char *argv[])
>   argv += optind;
>
>   if (Pflag) {
> - if (pledge("stdio rpath wpath cpath getpw", NULL) == -1)
> + if (pledge("stdio rpath wpath cpath fattr getpw", NULL) == -1)
>   err(1, "pledge");
>   } else {
>   if (pledge("stdio rpath cpath getpw", NULL) == -1)
> @@ -213,9 +213,9 @@ rm_tree(char **argv)
>
>   case FTS_F:
>   case FTS_NSOK:
> - if (Pflag)
> - rm_overwrite(p->fts_accpath, p->fts_info ==
> - FTS_NSOK ? NULL : p->fts_statp);
> + if (Pflag && !rm_overwrite(p->fts_accpath,
> + p->fts_info == FTS_NSOK ? NULL : p->fts_statp))
> + continue;
>   /* FALLTHROUGH */
>   default:
>   if (!unlink(p->fts_accpath) ||
> @@ -264,8 +264,8 @@ rm_file(char **argv)
>   else if (S_ISDIR(sb.st_mode))
>   rval = rmdir(f);
>   else {
> - if (Pflag)
> - rm_overwrite(f, );
> + if (Pflag && !rm_overwrite(f, ))
> + continue;
>   rval = unlink(f);
>   }
>   if (rval && (!fflag || errno != ENOENT)) {
> @@ -308,9 +308,12 @@ rm_overwrite(char *file, struct stat *sb
>   if (sbp->st_nlink > 1) {
>   warnx("%s (inode %llu): not overwritten due to multiple links",
>   file, (unsigned long long)sbp->st_ino);
> - return (0);
> + return (1);
>   }
> - if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1)
> + if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1 &&
> + (errno != EACCES ||
> +  chmod(file, sb.st_mode | S_IWUSR) == -1 ||
> +  (fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1))
>   goto err;
>   if (fstat(fd, ))
>   goto err;

Introduce fd_iterfile()

[Martin Pieuchot]

Diff below introduces a new helper function to iterate on `filehead'. 

This global data structures contains a reference to living 'struct file *'
so its access must be serialized with fdrop().  This is what the comment
below explains.  Currently the serialization is done by the KERNEL_LOCK(),
but since I'd like to remove the KERNEL_LOCK() from syscalls that call
FRELE(9) we'll need to use another lock.  So having all the accesses to
`filehead' in the same place makes it easier.

Ok?


Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.150
diff -u -p -r1.150 kern_descrip.c
--- kern/kern_descrip.c 12 Apr 2018 10:30:18 -  1.150
+++ kern/kern_descrip.c 17 Apr 2018 07:51:51 -
@@ -180,6 +180,28 @@ fd_unused(struct filedesc *fdp, int fd)
 }
 
 struct file *
+fd_iterfile(struct file *fp, struct proc *p)
+{
+   struct file *nfp;
+
+   if (fp == NULL)
+   nfp = LIST_FIRST();
+   else
+   nfp = LIST_NEXT(fp, f_list);
+
+   /* don't FREF when f_count == 0 to avoid race in fdrop() */
+   while (nfp != NULL && nfp->f_count == 0)
+   nfp = LIST_NEXT(nfp, f_list);
+   if (nfp != NULL)
+   FREF(nfp);
+
+   if (fp != NULL)
+   FRELE(fp, p);
+
+   return nfp;
+}
+
+struct file *
 fd_getfile(struct filedesc *fdp, int fd)
 {
struct file *fp;
Index: kern/kern_sysctl.c
===
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.332
diff -u -p -r1.332 kern_sysctl.c
--- kern/kern_sysctl.c  19 Feb 2018 08:59:52 -  1.332
+++ kern/kern_sysctl.c  16 Apr 2018 12:43:19 -
@@ -1250,7 +1250,7 @@ sysctl_file(int *name, u_int namelen, ch
 {
struct kinfo_file *kf;
struct filedesc *fdp;
-   struct file *fp, *nfp;
+   struct file *fp;
struct process *pr;
size_t buflen, elem_size, elem_count, outsize;
char *dp = where;
@@ -1318,14 +1318,8 @@ sysctl_file(int *name, u_int namelen, ch
 #endif
NET_UNLOCK();
}
-   fp = LIST_FIRST();
-   /* don't FREF when f_count == 0 to avoid race in fdrop() */
-   while (fp != NULL && fp->f_count == 0)
-   fp = LIST_NEXT(fp, f_list);
-   if (fp == NULL)
-   break;
-   FREF(fp);
-   do {
+   fp = NULL; ;
+   while ((fp = fd_iterfile(fp, p)) != NULL) {
if (fp->f_count > 1 && /* 0, +1 for our FREF() */
FILE_IS_USABLE(fp) &&
(arg == 0 || fp->f_type == arg)) {
@@ -1339,14 +1333,7 @@ sysctl_file(int *name, u_int namelen, ch
if (!skip)
FILLIT(fp, NULL, 0, NULL, NULL);
}
-   nfp = LIST_NEXT(fp, f_list);
-   while (nfp != NULL && nfp->f_count == 0)
-   nfp = LIST_NEXT(nfp, f_list);
-   if (nfp != NULL)
-   FREF(nfp);
-   FRELE(fp, p);
-   fp = nfp;
-   } while (fp != NULL);
+   };
break;
case KERN_FILE_BYPID:
/* A arg of -1 indicates all processes */
Index: sys/file.h
===
RCS file: /cvs/src/sys/sys/file.h,v
retrieving revision 1.40
diff -u -p -r1.40 file.h
--- sys/file.h  10 Feb 2018 05:24:23 -  1.40
+++ sys/file.h  16 Apr 2018 12:43:19 -
@@ -106,7 +106,6 @@ struct file {
 intfdrop(struct file *, struct proc *);
 
 LIST_HEAD(filelist, file);
-extern struct filelist filehead;   /* head of list of open files */
 extern int maxfiles;   /* kernel limit on number of open files 
*/
 extern int numfiles;   /* actual number of open files */
 extern struct fileops vnops;   /* vnode operations for files */
Index: sys/filedesc.h
===
RCS file: /cvs/src/sys/sys/filedesc.h,v
retrieving revision 1.34
diff -u -p -r1.34 filedesc.h
--- sys/filedesc.h  11 Feb 2017 19:51:06 -  1.34
+++ sys/filedesc.h  16 Apr 2018 12:43:19 -
@@ -133,6 +133,7 @@ voidfdfree(struct proc *p);
 intfdrelease(struct proc *p, int);
 void   fdremove(struct filedesc *, int);
 void   fdcloseexec(struct proc *);
+struct file *fd_iterfile(struct file *, struct proc *);
 struct file *fd_getfile(struct filedesc *, int);
 struct file *fd_getfile_mode(struct filedesc *, int, int);
 

Re: FREF(9) in unp_internalize()

[Martin Pieuchot]

On 16/04/18(Mon) 09:09, Todd C. Miller wrote:
> On Mon, 16 Apr 2018 10:19:40 +0200, Martin Pieuchot wrote:
> 
> > Diff below does FREF(9) earlier instead of incrementing `f_count' by hand.
> >
> > The error path is also updated to call FRELE(9) accordingly.
> 
> Wouldn't it be less error prone to simply add:
> 
>   if (fp != NULL)
>   FRELE(fp, p);
> 
> to the fail label?  If we get to fail, fp is either NULL or needs to
> drop a reference.

Sure, here's an updated diff.  It also moves the FRELE(9) in the error
loop down as suggested by visa@.

Index: kern/uipc_usrreq.c
===
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.123
diff -u -p -r1.123 uipc_usrreq.c
--- kern/uipc_usrreq.c  4 Jan 2018 10:45:30 -   1.123
+++ kern/uipc_usrreq.c  17 Apr 2018 07:48:09 -
@@ -838,6 +838,7 @@ morespace:
error = EBADF;
goto fail;
}
+   FREF(fp);
if (fp->f_count == LONG_MAX-2) {
error = EDEADLK;
goto fail;
@@ -845,7 +846,7 @@ morespace:
error = pledge_sendfd(p, fp);
if (error)
goto fail;
-   
+
/* kqueue descriptors cannot be copied */
if (fp->f_type == DTYPE_KQUEUE) {
error = EINVAL;
@@ -854,7 +855,6 @@ morespace:
rp->fp = fp;
rp->flags = fdp->fd_ofileflags[fd] & UF_PLEDGED;
rp--;
-   fp->f_count++;
if ((unp = fptounp(fp)) != NULL) {
unp->unp_file = fp;
unp->unp_msgcount++;
@@ -863,13 +863,15 @@ morespace:
}
return (0);
 fail:
+   if (fp != NULL)
+   FRELE(fp, p);
/* Back out what we just did. */
for ( ; i > 0; i--) {
rp++;
fp = rp->fp;
-   fp->f_count--;
if ((unp = fptounp(fp)) != NULL)
unp->unp_msgcount--;
+   FRELE(fp, p);
unp_rights--;
}
 

Re: vmd: enable pause/unpause for vm owners

[Pratik Vyas]

* Mohamed Aslan  [2018-04-16 00:54:43 -0400]:


Hello tech@,

I noticed that vmd(8) only allows VM owners to start/stop their
VMs, but does not let them to pause/unpause those VMs.

I was just wondering if there are reasons behind that. If not, the
patch below enables pause/unpause commands for VM owners.

Regards,
Aslan


Hi Aslan,

No reason behind not letting owners pause / unpause.  vmctl send
/ receive is also missing this and it's on my list.

Thanks for the patch!  It looks ok and I will commit it.

--
Pratik