From owner-freebsd-hackers Fri Jun 9 16:44:15 2000 Delivered-To: freebsd-hackers@freebsd.org Received: from fw.wintelcom.net (ns1.wintelcom.net [209.1.153.20]) by hub.freebsd.org (Postfix) with ESMTP id A2CC337C1F2; Fri, 9 Jun 2000 16:44:08 -0700 (PDT) (envelope-from bright@fw.wintelcom.net) Received: (from bright@localhost) by fw.wintelcom.net (8.10.0/8.10.0) id e59Ni8311226; Fri, 9 Jun 2000 16:44:08 -0700 (PDT) Date: Fri, 9 Jun 2000 16:44:08 -0700 From: Alfred Perlstein To: hackers@freebsd.org Cc: green@freebsd.org Subject: uidinfo has many race conditions. Message-ID: <20000609164408.N18462@fw.wintelcom.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2i Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG hi, Is it just me or does the fact that uidinfo structures (see kern/kern_proc.c) are allocated with M_WAITOK after finding them fails and then inserted into the uidhash structure a race condition? There's also a problem with sbsize checking because of races going on here, what needs to happen is that the changeXXsize/count functions need to know what they are chenging and doing them without races. I should have some diffs up soon that address this. here's fixing the uidinfo stuff, but it still doesn't fix that the sbsize checks before setting the size, which needs to be done atomically. I think the solution is to have chgsbsize actually check the rlimit instead of polling then setting. chblob is for my own stuff. Index: kern_proc.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_proc.c,v retrieving revision 1.63 diff -u -u -r1.63 kern_proc.c --- kern_proc.c 2000/02/08 19:54:15 1.63 +++ kern_proc.c 2000/06/09 23:40:46 @@ -56,6 +56,9 @@ MALLOC_DEFINE(M_SUBPROC, "subproc", "Proc sub-structures"); static void pgdelete __P((struct pgrp *)); +static struct uidinfo *uifind(uid_t uid); +static struct uidinfo *uicreate(uid_t uid); +static int uifree(struct uidinfo *uip); /* * Structure associated with user cacheing. @@ -65,6 +68,7 @@ uid_t ui_uid; long ui_proccnt; rlim_t ui_sbsize; + rlim_t ui_blobsize; }; #define UIHASH(uid) (&uihashtbl[(uid) & uihash]) static LIST_HEAD(uihashhead, uidinfo) *uihashtbl; @@ -99,6 +103,57 @@ } /* + * find/create a uidinfo struct for the uid passed in + */ +static struct uidinfo * +uifind(uid_t uid) +{ + struct uihashhead *uipp; + struct uidinfo *uip; + + uipp = UIHASH(uid); + LIST_FOREACH(uip, uipp, ui_hash) + if (uip->ui_uid == uid) + break; + + return (uip); +} + +static struct uidinfo * +uicreate(uid_t uid) +{ + struct uidinfo *uip, *norace; + + MALLOC(uip, struct uidinfo *, sizeof(*uip), M_PROC, M_WAITOK); + /* + * if we M_WAITOK we must look afterwards or risk redundant entries + */ + norace = uifind(uid); + if (norace != NULL) { + FREE(uip, M_PROC); + return (norace); + } + uip->ui_uid = uid; + uip->ui_proccnt = 0; + uip->ui_sbsize = 0; + uip->ui_blobsize = 0; + return (uip); +} + +static int +uifree(struct uidinfo *uip) +{ + + if (uip->ui_sbsize == 0 && uip->ui_proccnt == 0 && uip->ui_blobsize == 0) { + LIST_REMOVE(uip, ui_hash); + FREE(uip, M_PROC); + return (1); + } + return (0); +} + + +/* * Change the count associated with number of processes * a given user is using. */ @@ -108,35 +163,25 @@ int diff; { register struct uidinfo *uip; - register struct uihashhead *uipp; - uipp = UIHASH(uid); - LIST_FOREACH(uip, uipp, ui_hash) - if (uip->ui_uid == uid) - break; + uip = uifind(uid); if (uip) { uip->ui_proccnt += diff; if (uip->ui_proccnt < 0) panic("chgproccnt: procs < 0"); - if (uip->ui_proccnt > 0 || uip->ui_sbsize > 0) - return (uip->ui_proccnt); - LIST_REMOVE(uip, ui_hash); - FREE(uip, M_PROC); - return (0); + return (uifree(uip) == 1 ? 0 : uip->ui_proccnt); } if (diff <= 0) { if (diff == 0) return(0); panic("chgproccnt: lost user"); } - MALLOC(uip, struct uidinfo *, sizeof(*uip), M_PROC, M_WAITOK); - LIST_INSERT_HEAD(uipp, uip, ui_hash); - uip->ui_uid = uid; + uip = uicreate(uid); uip->ui_proccnt = diff; - uip->ui_sbsize = 0; return (diff); } + /* * Change the total socket buffer size a user has used. */ @@ -146,12 +191,8 @@ rlim_t diff; { register struct uidinfo *uip; - register struct uihashhead *uipp; - uipp = UIHASH(uid); - LIST_FOREACH(uip, uipp, ui_hash) - if (uip->ui_uid == uid) - break; + uip = uifind(uid); if (diff <= 0) { if (diff == 0) return (uip ? uip->ui_sbsize : 0); @@ -159,20 +200,38 @@ } if (uip) { uip->ui_sbsize += diff; - if (uip->ui_sbsize == 0 && uip->ui_proccnt == 0) { - LIST_REMOVE(uip, ui_hash); - FREE(uip, M_PROC); - return (0); - } - return (uip->ui_sbsize); + return (uifree(uip) == 1 ? 0 : uip->ui_sbsize); } - MALLOC(uip, struct uidinfo *, sizeof(*uip), M_PROC, M_WAITOK); - LIST_INSERT_HEAD(uipp, uip, ui_hash); - uip->ui_uid = uid; - uip->ui_proccnt = 0; + uip = uicreate(uid); uip->ui_sbsize = diff; return (diff); } + +/* + * Change the total amount of kblob space a suser can consume + */ +rlim_t +chgblobsize(uid, diff) + uid_t uid; + rlim_t diff; +{ + register struct uidinfo *uip; + + uip = uifind(uid); + if (diff <= 0) { + if (diff == 0) + return (uip ? uip->ui_blobsize : 0); + KASSERT(uip != NULL, ("uidinfo (%d) gone", uid)); + } + if (uip) { + uip->ui_blobsize += diff; + return (uifree(uip) == 1 ? 0 : uip->ui_blobsize); + } + uicreate(uid); + uip->ui_blobsize = diff; + return (diff); +} + /* * Is p an inferior of the current process? To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message