Re: FREF(9) in unp_internalize()
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--; }
Introduce fd_iterfile()
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(&filehead); + 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(&filehead); - /* 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: Should rm(1) -Pf change file permission?
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, &sb); > + if (Pflag && !rm_overwrite(f, &sb)) > + 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, &sb2)) > goto err;
Re: FREF(9) in unp_internalize()
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: Kernel size beyond 16 MB on amd64
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: Should rm(1) -Pf change file permission?
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: Should rm(1) -Pf change file permission?
Ingo Schwarze writes: Hello Ingo, > 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. Ok. > The conversation resulted in a clarification in the manual page, > though. Thanks for the clarification. It's clear now it's a "best effort". >> 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. Ok. So, this issue is closed. :) Best, > Yours, > Ingo