Subject: Re: UVM patch: sys_swapctl split
To: Emmanuel Dreyfus <manu@netbsd.org>
From: Chuck Silvers <chuq@chuq.com>
List: tech-kern
Date: 03/17/2002 11:22:21
hi,
some comments:
- you shouldn't just blindly malloc() the amount of memory that the
user asked for, since the user could pass in some really large number
which could cause the system to panic. a better way to do this would be
to pass in a pointer to a function with the signature of copyout() to
uvm_swap_stats(), and call that instead of copyout() or memcpy().
for sys_swapctl() you'd pass the real copyout(), for your compat version
you'd pass a new function that would reformat the entry to whatever
structure you need and copyout() the reformatted version instead.
- if you use UVMHIST_LOG() in a function, there must be UVMHIST_FUNC()
and UVMHIST_CALLED() at the top. look at the definitions of these
if you want to know why. I'd say it's ok to just remove all the UVMHIST
stuff from sys_swapctl(), it doesn't provide any useful info anyway.
- to use a pointer to a structure without having the structure definition
available, you can just declare the structure without saying what's in it.
just put "struct swapent;" before the prototype for uvm_swap_stats().
-Chuck
On Sun, Mar 17, 2002 at 07:27:19PM +0100, Emmanuel Dreyfus wrote:
> After the recent dicsussions on the stackgap problem with swapctl(SWAP_STATS)
> emulation for IRIX, I come to the following patch. I've got a few questions at
> the end...
>
> Index: uvm_swap.c
> ===================================================================
> RCS file: /cvsroot/syssrc/sys/uvm/uvm_swap.c,v
> retrieving revision 1.60
> diff -U2 -r1.60 uvm_swap.c
> --- uvm_swap.c 2002/03/09 07:28:20 1.60
> +++ uvm_swap.c 2002/03/17 18:21:32
> @@ -457,5 +457,5 @@
> char userpath[PATH_MAX + 1];
> size_t len;
> - int count, error, misc;
> + int error, misc;
> int priority;
> UVMHIST_FUNC("sys_swapctl"); UVMHIST_CALLED(pdhist);
> @@ -495,51 +495,11 @@
> #endif
> ) {
> - sep = (struct swapent *)SCARG(uap, arg);
> - count = 0;
> + len = sizeof(struct swapent) * misc;
> + sep = (struct swapent *)malloc(len, M_TEMP, M_WAITOK);
>
> - LIST_FOREACH(spp, &swap_priority, spi_swappri) {
> - for (sdp = CIRCLEQ_FIRST(&spp->spi_swapdev);
> - sdp != (void *)&spp->spi_swapdev && misc-- > 0;
> - sdp = CIRCLEQ_NEXT(sdp, swd_next)) {
> - /*
> - * backwards compatibility for system call.
> - * note that we use 'struct oswapent' as an
> - * overlay into both 'struct swapdev' and
> - * the userland 'struct swapent', as we
> - * want to retain backwards compatibility
> - * with NetBSD 1.3.
> - */
> - sdp->swd_ose.ose_inuse =
> - btodb((u_int64_t)sdp->swd_npginuse <<
> - PAGE_SHIFT);
> - error = copyout(&sdp->swd_ose, sep,
> - sizeof(struct oswapent));
> + uvm_swap_stats(SCARG(uap, cmd), (void *)sep, misc, retval);
> + error = copyout(sep, (void *)SCARG(uap, arg), len);
> + free(sep, M_TEMP);
>
> - /* now copy out the path if necessary */
> -#if defined(COMPAT_13)
> - if (error == 0 && SCARG(uap, cmd) == SWAP_STATS)
> -#else
> - if (error == 0)
> -#endif
> - error = copyout(sdp->swd_path,
> - &sep->se_path, sdp->swd_pathlen);
> -
> - if (error)
> - goto out;
> - count++;
> -#if defined(COMPAT_13)
> - if (SCARG(uap, cmd) == SWAP_OSTATS)
> - sep = (struct swapent *)
> - ((struct oswapent *)sep + 1);
> - else
> -#endif
> - sep++;
> - }
> - }
> -
> - UVMHIST_LOG(pdhist, "<- done SWAP_STATS", 0, 0, 0, 0);
> -
> - *retval = count;
> - error = 0;
> goto out;
> }
> @@ -716,4 +676,65 @@
> UVMHIST_LOG(pdhist, "<- done! error=%d", error, 0, 0, 0);
> return (error);
> +}
> +
> +/*
> + * swap_stats: implements swapctl(SWAP_STATS). The function is kept
> + * away from sys_swapctl() in order to allow COMPAT_* swapctl()
> + * emulation to use it directly without going through sys_swapctl().
> + * The problem with using sys_swapctl() there is that it involves
> + * copying the swapent array to the stackgap, and this array's size
> + * is not known at build time. Hence it would not be possible to
> + * ensure it would fit in the stackgap in any case.
> + */
> +void
> +uvm_swap_stats(cmd, sep, sec, retval)
> + int cmd;
> + void *sep;
> + int sec;
> + register_t *retval;
> +{
> + struct swappri *spp;
> + struct swapdev *sdp;
> + int count = 0;
> +
> + LIST_FOREACH(spp, &swap_priority, spi_swappri) {
> + for (sdp = CIRCLEQ_FIRST(&spp->spi_swapdev);
> + sdp != (void *)&spp->spi_swapdev && sec-- > 0;
> + sdp = CIRCLEQ_NEXT(sdp, swd_next)) {
> + /*
> + * backwards compatibility for system call.
> + * note that we use 'struct oswapent' as an
> + * overlay into both 'struct swapdev' and
> + * the userland 'struct swapent', as we
> + * want to retain backwards compatibility
> + * with NetBSD 1.3.
> + */
> + sdp->swd_ose.ose_inuse =
> + btodb((u_int64_t)sdp->swd_npginuse <<
> + PAGE_SHIFT);
> + (void)memcpy(sep, &sdp->swd_ose,
> + sizeof(struct oswapent));
> +
> + /* now copy out the path if necessary */
> +#if defined(COMPAT_13)
> + if (SCARG(uap, cmd) == SWAP_STATS)
> +#endif
> + (void)memcpy(&((struct swapent *)sep)->se_path,
> + sdp->swd_path, sdp->swd_pathlen);
> +
> + count++;
> +#if defined(COMPAT_13)
> + if (SCARG(uap, cmd) == SWAP_OSTATS)
> + sep = (void *)((struct oswapent *)sep + 1);
> + else
> +#endif
> + ((struct swapent *)sep)++;
> + }
> + }
> +
> + UVMHIST_LOG(pdhist, "<- done SWAP_STATS", 0, 0, 0, 0);
> +
> + *retval = count;
> + return;
> }
>
> Index: uvm_swap.h
> ===================================================================
> RCS file: /cvsroot/syssrc/sys/uvm/uvm_swap.h,v
> retrieving revision 1.5
> diff -U2 -r1.5 uvm_swap.h
> --- uvm_swap.h 2000/01/11 06:57:51 1.5
> +++ uvm_swap.h 2002/03/17 18:21:33
> @@ -43,4 +43,5 @@
> void uvm_swap_free __P((int, int));
> void uvm_swap_markbad __P((int, int));
> +void uvm_swap_stats __P((int, void *, int, register_t *));
>
> #endif /* _KERNEL */
>
> There is one problem I have not been able to address yet: I started with
> void uvm_swap_stats __P((int, struct swapent *, int, register_t *));
> but this makes an error at build time:
>
>
> cat ../../../../arch/mips/mips/genassym.cf | sh ../../../../kern/genassym.sh
> cc -EB -G 0 -mno-abicalls -mno-half-pic -ffreestanding -g -DDEBUG_IRIX -O2
> -Werror -Wall -Wno-main -Wpointer-arith -Wmissing-prototypes -Wstrict-prototypes
> -Wno-uninitialized -Dsgimips -I. -I../../../../arch -I../../../.. -nostdinc
> -DMIPS3_ENABLE_CLOCK_INTR -DSCSI_DELAY="5" -DMIPS3 -DPARANOIADIAG -DDEBUG
> -DDIAGNOSTIC -DMAXUSERS=32 -D_KERNEL -D_KERNEL_OPT > assym.h.tmp && mv -f
> assym.h.tmp assym.h
> cc1: warnings being treated as errors
> In file included from ../../../../uvm/uvm.h:65,
> from /tmp/332.c:11:
> ../../../../uvm/uvm_swap.h:45: warning: `struct swapent' declared inside
> parameter list
> ../../../../uvm/uvm_swap.h:45: warning: its scope is only this definition or
> declaration,
> ../../../../uvm/uvm_swap.h:45: warning: which is probably not what you want.
>
>
> I had two solutions: adding an #include "sys/swap.h" in uvm/uvm_swap.h or
> changing arg #2 type to void * in uvm_swap_stats() prototype. What is the
> prefered way?
>
> Also: should I UVMHIST_LOG the entry to uvm_swap_stats?
>
> --
> Emmanuel Dreyfus
> manu@netbsd.org