Date: Tue, 18 Sep 2001 18:58:58 -0500 (CDT) From: "Andrew P. Lentvorski" <andrewl@io.com> To: <FreeBSD-gnats-submit@freebsd.org> Cc: <andrewl@io.com> Subject: bin/30661: FreeBSD-current fails to do partial NFS file locking Message-ID: <Pine.LNX.4.33.0109181852150.26837-100000@hagbard.io.com>
next in thread | raw e-mail | index | archive | help
>Number: 30661 >Category: bin >Synopsis: FreeBSD-current fails to do partial NFS file locking >Confidential: no >Severity: non-critical >Priority: medium >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Tue Sep 18 17:00:02 PDT 2001 >Closed-Date: >Last-Modified: >Originator: Andrew Lentvorski >Release: FreeBSD 5.0-CURRENT i386 >Organization: North Shore Circuit Design >Environment: System: FreeBSD daffy.buzmeg.com 5.0-CURRENT FreeBSD 5.0-CURRENT #0: Sun Sep 16 18:43:22 PDT 2001 root@daffy.buzmeg.com:/usr/obj/usr/src/sys/GENERIC i386 >Description: FreeBSD-CURRENT fails to do partial-file NFS locking >How-To-Repeat: Run the Connectathon 2001 NFS suites (http://www.connectathon.org/nfstests.html) >Fix: Apply the following patch to enable partial-file NFS lock testing --- lockd.patches begins here --- diff -crN /usr/src/usr.sbin/rpc.lockd/lock_proc.c rpc.lockd/lock_proc.c *** /usr/src/usr.sbin/rpc.lockd/lock_proc.c Fri Apr 27 21:26:32 2001 --- rpc.lockd/lock_proc.c Tue Sep 18 15:34:38 2001 *************** *** 87,92 **** --- 87,131 ---- syslog(LOG_DEBUG, "%s from %s", fun_name, hostname_buf); } + /* log_netobj ----------------------------------------------------------- */ + /* + Purpose: Log a netobj + Returns: Nothing + Notes: This function should only really be called as part of + a debug subsystem. + */ + + static void log_netobj(netobj *obj) + { + char objvalbuffer[(sizeof(char)*2)*MAX_NETOBJ_SZ+2]; + char objascbuffer[sizeof(char)*MAX_NETOBJ_SZ+1]; + int i; + int maxlen; + char *tmp1; + char *tmp2; + + /* Notify of potential security attacks */ + if (obj->n_len > MAX_NETOBJ_SZ) + { + syslog(LOG_DEBUG, "SOMEONE IS TRYING TO DO SOMETHING NASTY!\n"); + syslog(LOG_DEBUG, "netobj too large! Should be %d was %d\n", MAX_NETOBJ_SZ, obj->n_len); + } + + /* Prevent the security hazard from the buffer overflow */ + maxlen = (obj->n_len < MAX_NETOBJ_SZ ? obj->n_len : MAX_NETOBJ_SZ); + for (i=0, tmp1=objvalbuffer, tmp2=objascbuffer; i < obj->n_len; i++, tmp1 +=2, tmp2 +=1) + { + sprintf(tmp1,"%02X",*(obj->n_bytes+i)); + sprintf(tmp2,"%c",*(obj->n_bytes+i)); + } + + sprintf(tmp1,"\0"); + sprintf(tmp2,"\0"); + + syslog(LOG_DEBUG,"netobjvals: %s\n",objvalbuffer); + syslog(LOG_DEBUG,"netobjascs: %s\n",objascbuffer); + } + /* get_client -------------------------------------------------------------- */ /* * Purpose: Get a CLIENT* for making RPC calls to lockd on given host *************** *** 382,388 **** if (debug_level) log_from_addr("nlm_test", rqstp); ! holder = testlock(&arg4, 0); /* * Copy the cookie from the argument into the result. Note that this * is slightly hazardous, as the structure contains a pointer to a --- 421,427 ---- if (debug_level) log_from_addr("nlm_test", rqstp); ! holder = testlock(&arg4, arg->exclusive, 0); /* * Copy the cookie from the argument into the result. Note that this * is slightly hazardous, as the structure contains a pointer to a *************** *** 422,428 **** if (debug_level) log_from_addr("nlm_test_msg", rqstp); ! holder = testlock(&arg4, 0); res.cookie = arg->cookie; if (holder == NULL) { --- 461,467 ---- if (debug_level) log_from_addr("nlm_test_msg", rqstp); ! holder = testlock(&arg4, arg->exclusive, 0); res.cookie = arg->cookie; if (holder == NULL) { *************** *** 865,874 **** static nlm4_testres res; struct nlm4_holder *holder; ! if (debug_level) log_from_addr("nlm4_test", rqstp); ! holder = testlock(&arg->alock, LOCK_V4); /* * Copy the cookie from the argument into the result. Note that this --- 904,931 ---- static nlm4_testres res; struct nlm4_holder *holder; ! if (debug_level) { log_from_addr("nlm4_test", rqstp); + } + + if (debug_level > 5) { + syslog(LOG_DEBUG, "Locking arguments:\n"); + log_netobj(&(arg->cookie)); + + syslog(LOG_DEBUG, "Alock arguments:\n"); + syslog(LOG_DEBUG, "Caller Name: %s\n",arg->alock.caller_name); + syslog(LOG_DEBUG, "File Handle:\n"); + log_netobj(&(arg->alock.fh)); + syslog(LOG_DEBUG, "Owner Handle:\n"); + log_netobj(&(arg->alock.oh)); + syslog(LOG_DEBUG, "SVID: %d\n", arg->alock.svid); + syslog(LOG_DEBUG, "Lock Offset: %d\n", arg->alock.l_offset); + syslog(LOG_DEBUG, "Lock Length: %d\n", arg->alock.l_len); + + syslog(LOG_DEBUG, "Exclusive: %s\n", (arg->exclusive ? "true" : "false")); + } ! holder = testlock(&arg->alock, arg->exclusive, LOCK_V4); /* * Copy the cookie from the argument into the result. Note that this *************** *** 904,910 **** if (debug_level) log_from_addr("nlm4_test_msg", rqstp); ! holder = testlock(&arg->alock, LOCK_V4); res.cookie = arg->cookie; if (holder == NULL) { --- 961,967 ---- if (debug_level) log_from_addr("nlm4_test_msg", rqstp); ! holder = testlock(&arg->alock, arg->exclusive, LOCK_V4); res.cookie = arg->cookie; if (holder == NULL) { *************** *** 946,953 **** { static nlm4_res res; ! if (debug_level) log_from_addr("nlm4_lock", rqstp); /* copy cookie from arg to result. See comment in nlm_test_4() */ res.cookie = arg->cookie; --- 1003,1031 ---- { static nlm4_res res; ! if (debug_level) { log_from_addr("nlm4_lock", rqstp); + } + + if (debug_level > 5) { + syslog(LOG_DEBUG, "Locking arguments:\n"); + log_netobj(&(arg->cookie)); + + syslog(LOG_DEBUG, "Alock arguments:\n"); + syslog(LOG_DEBUG, "Caller Name: %s\n",arg->alock.caller_name); + syslog(LOG_DEBUG, "File Handle:\n"); + log_netobj(&(arg->alock.fh)); + syslog(LOG_DEBUG, "Owner Handle:\n"); + log_netobj(&(arg->alock.oh)); + syslog(LOG_DEBUG, "SVID: %d\n", arg->alock.svid); + syslog(LOG_DEBUG, "Lock Offset: %d\n", arg->alock.l_offset); + syslog(LOG_DEBUG, "Lock Length: %d\n", arg->alock.l_len); + + syslog(LOG_DEBUG, "Block: %s\n", (arg->block ? "true" : "false")); + syslog(LOG_DEBUG, "Exclusive: %s\n", (arg->exclusive ? "true" : "false")); + syslog(LOG_DEBUG, "Reclaim: %s\n", (arg->reclaim ? "true" : "false")); + syslog(LOG_DEBUG, "State num: %d\n", arg->state); + } /* copy cookie from arg to result. See comment in nlm_test_4() */ res.cookie = arg->cookie; diff -crN /usr/src/usr.sbin/rpc.lockd/lockd_lock.c rpc.lockd/lockd_lock.c *** /usr/src/usr.sbin/rpc.lockd/lockd_lock.c Tue Apr 17 13:45:23 2001 --- rpc.lockd/lockd_lock.c Tue Sep 18 15:34:38 2001 *************** *** 54,59 **** --- 54,70 ---- #include "lockd_lock.h" #include "lockd.h" + + /* Why was this file written in K&R C? -- APL 09/17/01 */ + + /* + The following data structure really needs to be something + with better search performance than a LIST. In fact, a LIST + is a particularly bad structure as you *always* have to traverse + the *entire* list in sequential fashion when attempting to do *anything* + with a lock. This is O(n). + */ + /* A set of utilities for managing file locking */ LIST_HEAD(lcklst_head, file_lock); struct lcklst_head lcklst_head = LIST_HEAD_INITIALIZER(lcklst_head); *************** *** 75,80 **** --- 86,92 ---- /* lock status */ #define LKST_LOCKED 1 /* lock is locked */ + /* FIXME: Check on this flag. Is this file specific or lock specific? */ #define LKST_WAITING 2 /* file is already locked by another host */ #define LKST_PROCESSING 3 /* child is trying to aquire the lock */ #define LKST_DYING 4 /* must dies when we get news from the child */ *************** *** 85,90 **** --- 97,103 ---- void send_granted __P((struct file_lock *, int)); void siglock __P((void)); void sigunlock __P((void)); + int regions_overlap(u_int64_t start1, u_int64_t len1, u_int64_t start2, u_int64_t len2); /* list of hosts we monitor */ LIST_HEAD(hostlst_head, host); *************** *** 100,114 **** void do_mon __P((char *)); /* * testlock(): inform the caller if the requested lock would be granted or not * returns NULL if lock would granted, or pointer to the current nlm4_holder * otherwise. */ struct nlm4_holder * ! testlock(lock, flags) ! struct nlm4_lock *lock; ! int flags; { struct file_lock *fl; fhandle_t filehandle; --- 113,157 ---- void do_mon __P((char *)); /* + * regions_overlap(): This function examines the two provided regions for overlap. + * It is non-trivial because start+len *CAN* overflow a 64-bit unsigned integer + * and NFS semantics are unspecified on this account. + */ + int + regions_overlap(u_int64_t start1, u_int64_t len1, u_int64_t start2, u_int64_t len2) + { + int result; + + result = FALSE; + + /* FIXME: Need to adjust checks to account for integer overflow */ + if (len1 == 0 && len2 == 0) { + /* Regions *must* overlap if they both extend to the end */ + result = TRUE; + } else if (len1 == 0 && start2+len2 < start1) { + /* Region 2 is completely to the left of Region 1 */ + result = FALSE; + } else if (start1+len1 < start2 && len2 == 0) { + /* Region 1 is completely to the left of region 2 */ + result = FALSE; + } else if (start1 + len1 <= start2 || start2+len2 <= start1) { + /* 1 is completely left of 2 or 2 is completely left of 1 */ + result = FALSE; + } else { + result = TRUE; + } + + return result; + } + + /* * testlock(): inform the caller if the requested lock would be granted or not * returns NULL if lock would granted, or pointer to the current nlm4_holder * otherwise. */ struct nlm4_holder * ! testlock(struct nlm4_lock *lock, bool_t exclusive, int flags) { struct file_lock *fl; fhandle_t filehandle; *************** *** 120,139 **** /* search through the list for lock holder */ for (fl = LIST_FIRST(&lcklst_head); fl != NULL; fl = LIST_NEXT(fl, lcklst)) { if (fl->status != LKST_LOCKED) continue; if (memcmp(&fl->filehandle, &filehandle, sizeof(filehandle))) continue; ! /* got it ! */ syslog(LOG_DEBUG, "test for %s: found lock held by %s", lock->caller_name, fl->client_name); - sigunlock(); return (&fl->client); } - /* not found */ - sigunlock(); - syslog(LOG_DEBUG, "test for %s: no lock found", lock->caller_name); - return NULL; } /* --- 163,200 ---- /* search through the list for lock holder */ for (fl = LIST_FIRST(&lcklst_head); fl != NULL; fl = LIST_NEXT(fl, lcklst)) { + if (fl->status != LKST_LOCKED) continue; + /* FIXME: Could we possibly have identical filehandles on different systems? */ + /* FIXME: ie. Do we need to check more than just the filehandle? */ + /* FIXME: ie. Could someone artificially create requests which are security violations? */ if (memcmp(&fl->filehandle, &filehandle, sizeof(filehandle))) continue; ! /* File handles match, look for lock region overlap */ ! if (regions_overlap(lock->l_offset, lock->l_len, ! fl->client.l_offset,fl->client.l_len)) { ! syslog(LOG_DEBUG, "Region overlap found %llu : %llu -- %llu : %llu\n", ! lock->l_offset, lock->l_len, ! fl->client.l_offset,fl->client.l_len); ! /* Regions overlap. Now check for exclusivity. */ ! if (exclusive || fl->client.exclusive) { ! /* Lock test must fail, regions are exclusive */ ! break; ! } ! } ! /* Continue looping through all locks */ ! } ! sigunlock(); ! ! if (fl == NULL) { ! syslog(LOG_DEBUG, "test for %s: no lock found", lock->caller_name); ! return NULL; ! } else { syslog(LOG_DEBUG, "test for %s: found lock held by %s", lock->caller_name, fl->client_name); return (&fl->client); } } /* diff -crN /usr/src/usr.sbin/rpc.lockd/lockd_lock.h rpc.lockd/lockd_lock.h *** /usr/src/usr.sbin/rpc.lockd/lockd_lock.h Mon Mar 19 04:50:09 2001 --- rpc.lockd/lockd_lock.h Tue Sep 18 15:34:38 2001 *************** *** 3,9 **** /* Headers and function declarations for file-locking utilities */ ! struct nlm4_holder * testlock __P((struct nlm4_lock *, int)); enum nlm_stats getlock __P((nlm4_lockargs *, struct svc_req *, int)); enum nlm_stats unlock __P((nlm4_lock *, int)); --- 3,9 ---- /* Headers and function declarations for file-locking utilities */ ! struct nlm4_holder * testlock __P((struct nlm4_lock *, int, int)); enum nlm_stats getlock __P((nlm4_lockargs *, struct svc_req *, int)); enum nlm_stats unlock __P((nlm4_lock *, int)); --- lockd.patches ends here --- --f8IMoaa01974.1000853436/daffy.buzmeg.com-- >Release-Note: >Audit-Trail: >Unformatted: X-send-pr-version: 3.113 X-GNATS-Notify: MIME-Version: 1.0 To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.LNX.4.33.0109181852150.26837-100000>