Date: Thu, 30 Aug 2007 23:35:59 +0800 From: "Tz-Huan Huang" <tzhuan@csie.org> To: "Bruce Evans" <brde@optusnet.com.au> Cc: Kostik Belousov <kostikbel@gmail.com>, bugs@freebsd.org, fs@freebsd.org Subject: Re: msdosfs not MPSAFE Message-ID: <6a7033710708300835n5ac95063o6331ede5fcc80662@mail.gmail.com> In-Reply-To: <20070814154812.J24186@besplex.bde.org> References: <20070712084115.GA2200@deviant.kiev.zoral.com.ua> <20070716195556.P12807@besplex.bde.org> <20070721063434.GI2200@deviant.kiev.zoral.com.ua> <20070721233613.Q3366@besplex.bde.org> <20070804075730.GP2738@deviant.kiev.zoral.com.ua> <20070808004001.O926@besplex.bde.org> <20070807170259.GJ2738@deviant.kiev.zoral.com.ua> <20070810133946.H769@besplex.bde.org> <20070810124153.GW2738@deviant.kiev.zoral.com.ua> <20070814154812.J24186@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] hi, I have a patch that remove the global variables nambuf_ptr, nambuf_len and nambuf_last_id entirely. It should be applied on recent 7-current. Please give it a try, thanks a lot. http://w.csie.org/~tzhuan/freebsd/msdosfs.diff Tz-Huan 2007/8/14, Bruce Evans <brde@optusnet.com.au>: > On Fri, 10 Aug 2007, Kostik Belousov wrote: > > > On Fri, Aug 10, 2007 at 01:54:48PM +1000, Bruce Evans wrote: > >> I wrote yet another patch, with allocation on the stack so that no locking > >> is required. This is simpler and doesn't require any new functions. > >> Unfortunately, it is larger because it changes the interfaces for most > >> functions. The interface changes are routine, so this is probably better. > >> Note that 'struct dirent's are already allocated on the stack. This > >> patch adds allocation of 'struct mbnambuf's which are slightly smaller > >> (~256 bytes). I think this is just small enough for stack allocation. > > > > I agree that this is the best approach. The size of the on-stack > > structure still make me worry, although ~270 bytes seems to be not too > > large for 3-pages stack. > > Stack growth seems to be nowhere near a problem. With the extra ~270 > bytes. ls -lR on i386 uses less than a 1-page stack (about 3.5K, > including 0x270 bytes for the pcb). I think the maximum stack depth > is attained when a debugger trap traps a fast interrupt interrupting > a page fault in an i/o routine called from msdosfs_readdir(). Unlike > in RELENG_4, nested interrupts can't occur, so a 1-page stack should > be enough for everything on i386 (but of course isn't quite enough). > I didn't try hard to attain the maximum depth, and just looked at where > the stack got to after running a large ls -lR for a while. msdosfs_readdir() > now allocates 0x2b0 bytes on the stack using "subl $0x2b0,%esp" and > that is now the largest single allocation. This is without INVARIANTS > etc. > > Bruce > _______________________________________________ > freebsd-fs@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org" > [-- Attachment #2 --] diff -ruN msdosfs.orig/direntry.h msdosfs/direntry.h --- msdosfs.orig/direntry.h 2006-10-24 19:14:05.000000000 +0800 +++ msdosfs/direntry.h 2007-08-30 15:14:04.000000000 +0800 @@ -136,18 +136,15 @@ struct dirent; struct msdosfsmount; -char *mbnambuf_flush(struct dirent *dp); -void mbnambuf_init(void); -void mbnambuf_write(char *name, int id); int dos2unixfn(u_char dn[11], u_char *un, int lower, struct msdosfsmount *pmp); int unix2dosfn(const u_char *un, u_char dn[12], size_t unlen, u_int gen, struct msdosfsmount *pmp); int unix2winfn(const u_char *un, size_t unlen, struct winentry *wep, int cnt, int chksum, struct msdosfsmount *pmp); -int winChkName(const u_char *un, size_t unlen, int chksum, +int winChkName(const u_char *un, size_t unlen, struct dirent *dirbufptr, int chksum, struct msdosfsmount *pmp); -int win2unixfn(struct winentry *wep, int chksum, struct msdosfsmount *pmp); +int win2unixfn(struct winentry *wep, struct dirent *dirbufptr, int chksum, struct msdosfsmount *pmp); u_int8_t winChksum(struct direntry *dep); int winSlotCnt(const u_char *un, size_t unlen, struct msdosfsmount *pmp); size_t winLenFixup(const u_char *un, size_t unlen); diff -ruN msdosfs.orig/msdosfs_conv.c msdosfs/msdosfs_conv.c --- msdosfs.orig/msdosfs_conv.c 2007-08-30 11:33:22.000000000 +0800 +++ msdosfs/msdosfs_conv.c 2007-08-30 16:57:15.000000000 +0800 @@ -64,13 +64,9 @@ static int mbsadjpos(const char **, size_t, size_t, int, int, void *handle); static u_int16_t dos2unixchr(const u_char **, size_t *, int, struct msdosfsmount *); static u_int16_t unix2doschr(const u_char **, size_t *, struct msdosfsmount *); -static u_int16_t win2unixchr(u_int16_t, struct msdosfsmount *); +static size_t win2unixchr(u_int16_t wc, struct dirent *dirbufptr, struct msdosfsmount *pmp); static u_int16_t unix2winchr(const u_char **, size_t *, int, struct msdosfsmount *); -static char *nambuf_ptr; -static size_t nambuf_len; -static int nambuf_last_id; - /* * 0 - character disallowed in long file name. * 1 - character should be replaced by '_' in DOS file name, @@ -249,6 +245,11 @@ int thislong = 0; u_int16_t c; +#ifdef MSDOSFS_DEBUG + u_char *un_ptr = un; + printf("dos2unixfn(): dn %s", dn); +#endif + /* * If first char of the filename is SLOT_E5 (0x05), then the real * first char of the filename should be 0xe5. But, they couldn't @@ -293,6 +294,10 @@ } *un++ = 0; +#ifdef MSDOSFS_DEBUG + printf(" -> un %s, size %d\n", un_ptr, thislong); +#endif + return (thislong); } @@ -601,37 +606,38 @@ * Returns the checksum or -1 if no match */ int -winChkName(un, unlen, chksum, pmp) +winChkName(un, unlen, dirbufptr, chksum, pmp) const u_char *un; size_t unlen; + struct dirent *dirbufptr; int chksum; struct msdosfsmount *pmp; { size_t len; u_int16_t c1, c2; u_char *np; - struct dirent dirbuf; - /* - * We alread have winentry in mbnambuf - */ - if (!mbnambuf_flush(&dirbuf) || !dirbuf.d_namlen) + if (!dirbufptr->d_namlen) return -1; #ifdef MSDOSFS_DEBUG - printf("winChkName(): un=%s:%d,d_name=%s:%d\n", un, unlen, - dirbuf.d_name, - dirbuf.d_namlen); + printf("winChkName(): un=0x"); + for (const u_char *c = un; *c; ++c) + printf("%02x", (unsigned char)*c); + printf(":%d, d_name=0x", unlen); + for (char *c = dirbufptr->d_name; *c; ++c) + printf("%02x", (unsigned char)*c); + printf(":%d\n", dirbufptr->d_namlen); #endif /* * Compare the name parts */ - len = dirbuf.d_namlen; + len = dirbufptr->d_namlen; if (unlen != len) return -2; - for (np = dirbuf.d_name; unlen > 0 && len > 0;) { + for (np = dirbufptr->d_name; unlen > 0 && len > 0;) { /* * Comparison must be case insensitive, because FAT disallows * to look up or create files in case sensitive even when @@ -646,19 +652,36 @@ } /* - * Convert Win95 filename to dirbuf. + * Reverse C-String + * for example, 12345 -> 54321 + */ +static void +strreverse(char *str, size_t n) +{ + char c; + size_t i; + for (i = 0; i < n/2; ++i) { + c = str[i]; + str[i] = str[n-i-1]; + str[n-i-1] = c; + } +} + +/* + * Convert Win95 filename to dirbuf (*dirbufptr). * Returns the checksum or -1 if impossible */ int -win2unixfn(wep, chksum, pmp) +win2unixfn(wep, dirbufptr, chksum, pmp) struct winentry *wep; + struct dirent *dirbufptr; int chksum; struct msdosfsmount *pmp; { u_int8_t *cp; - u_int8_t *np, name[WIN_CHARS * 2 + 1]; u_int16_t code; int i; + const size_t d_namlen = dirbufptr->d_namlen; /* save for reverse */ if ((wep->weCnt&WIN_CNT) > howmany(WIN_MAXLEN, WIN_CHARS) || !(wep->weCnt&WIN_CNT)) @@ -677,22 +700,18 @@ /* * Convert the name parts */ - np = name; for (cp = wep->wePart1, i = sizeof(wep->wePart1)/2; --i >= 0;) { code = (cp[1] << 8) | cp[0]; switch (code) { case 0: - *np = '\0'; - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1); + dirbufptr->d_name[dirbufptr->d_namlen] = '\0'; return chksum; case '/': - *np = '\0'; + dirbufptr->d_name[dirbufptr->d_namlen] = '\0'; return -1; default: - code = win2unixchr(code, pmp); - if (code & 0xff00) - *np++ = code >> 8; - *np++ = code; + code = win2unixchr(code, dirbufptr, pmp); + if (code) return -1; break; } cp += 2; @@ -701,17 +720,14 @@ code = (cp[1] << 8) | cp[0]; switch (code) { case 0: - *np = '\0'; - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1); + dirbufptr->d_name[dirbufptr->d_namlen] = '\0'; return chksum; case '/': - *np = '\0'; + dirbufptr->d_name[dirbufptr->d_namlen] = '\0'; return -1; default: - code = win2unixchr(code, pmp); - if (code & 0xff00) - *np++ = code >> 8; - *np++ = code; + code = win2unixchr(code, dirbufptr, pmp); + if (code) return -1; break; } cp += 2; @@ -720,23 +736,25 @@ code = (cp[1] << 8) | cp[0]; switch (code) { case 0: - *np = '\0'; - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1); + dirbufptr->d_name[dirbufptr->d_namlen] = '\0'; return chksum; case '/': - *np = '\0'; + dirbufptr->d_name[dirbufptr->d_namlen] = '\0'; return -1; default: - code = win2unixchr(code, pmp); - if (code & 0xff00) - *np++ = code >> 8; - *np++ = code; + code = win2unixchr(code, dirbufptr, pmp); + if (code) return -1; break; } cp += 2; } - *np = '\0'; - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1); + dirbufptr->d_name[dirbufptr->d_namlen] = '\0'; + + /* swap the dirbufptr->d_name */ + strreverse(dirbufptr->d_name, dirbufptr->d_namlen); + strreverse(dirbufptr->d_name, dirbufptr->d_namlen - d_namlen); + strreverse(dirbufptr->d_name + dirbufptr->d_namlen - d_namlen, d_namlen); + return chksum; } @@ -952,10 +970,10 @@ /* * Convert Windows char to Local char */ -static u_int16_t -win2unixchr(u_int16_t wc, struct msdosfsmount *pmp) +static size_t +win2unixchr(u_int16_t wc, struct dirent *dirbufptr, struct msdosfsmount *pmp) { - u_char *inp, *outp, inbuf[3], outbuf[3]; + u_char *inp, *outp, inbuf[3]; size_t ilen, olen, len; if (wc == 0) @@ -966,31 +984,49 @@ inbuf[1] = (u_char)wc; inbuf[2] = '\0'; - ilen = olen = len = 2; + ilen = 2; + olen = len = sizeof(dirbufptr->d_name) - dirbufptr->d_namlen - 1; inp = inbuf; - outp = outbuf; + outp = dirbufptr->d_name + dirbufptr->d_namlen; + msdosfs_iconv->convchr(pmp->pm_w2u, (const char **)&inp, &ilen, (char **)&outp, &olen); - len -= olen; - /* - * return '?' if failed to convert - */ - if (len == 0) { - wc = '?'; - return (wc); - } + dirbufptr->d_namlen += len - olen; - wc = 0; - while(len--) - wc |= (*(outp - len - 1) & 0xff) << (len << 3); - return (wc); + if (ilen != 0) { + /* failed to convert */ + if (olen == 0) { + /* filename longer then MAXNAMLEN */ + goto too_long; + } else { + /* some character cannot be converted */ + goto failed; + } + } + + goto ok; } - if (wc & 0xff00) - wc = '?'; + if (sizeof(dirbufptr->d_name) <= dirbufptr->d_namlen + 2) { + /* filename longer then MAXNAMLEN */ + goto too_long; + } else if (wc & 0xff00) { + goto failed; + } + dirbufptr->d_name[dirbufptr->d_namlen++] = (u_char)(wc>>8); + dirbufptr->d_name[dirbufptr->d_namlen++] = (u_char)wc; + goto ok; - return (wc); +ok: + return 0; + +too_long: + return 1; + +failed: + dirbufptr->d_name[dirbufptr->d_namlen++] = '?'; + return 2; } /* @@ -1035,78 +1071,3 @@ (*instr)++; return (wc); } - -/* - * Initialize the temporary concatenation buffer (once) and mark it as - * empty on subsequent calls. - */ -void -mbnambuf_init(void) -{ - - if (nambuf_ptr == NULL) { - nambuf_ptr = malloc(MAXNAMLEN + 1, M_MSDOSFSMNT, M_WAITOK); - nambuf_ptr[MAXNAMLEN] = '\0'; - } - nambuf_len = 0; - nambuf_last_id = -1; -} - -/* - * Fill out our concatenation buffer with the given substring, at the offset - * specified by its id. Since this function must be called with ids in - * descending order, we take advantage of the fact that ASCII substrings are - * exactly WIN_CHARS in length. For non-ASCII substrings, we shift all - * previous (i.e. higher id) substrings upwards to make room for this one. - * This only penalizes portions of substrings that contain more than - * WIN_CHARS bytes when they are first encountered. - */ -void -mbnambuf_write(char *name, int id) -{ - size_t count; - char *slot; - - KASSERT(nambuf_len == 0 || id == nambuf_last_id - 1, - ("non-decreasing id, id %d last id %d", id, nambuf_last_id)); - - /* Store this substring in a WIN_CHAR-aligned slot. */ - slot = nambuf_ptr + (id * WIN_CHARS); - count = strlen(name); - if (nambuf_len + count > MAXNAMLEN) { - printf("msdosfs: file name %zu too long\n", nambuf_len + count); - return; - } - - /* Shift suffix upwards by the amount length exceeds WIN_CHARS. */ - if (count > WIN_CHARS && nambuf_len != 0) - bcopy(slot + WIN_CHARS, slot + count, nambuf_len); - - /* Copy in the substring to its slot and update length so far. */ - bcopy(name, slot, count); - nambuf_len += count; - nambuf_last_id = id; -} - -/* - * Take the completed string and use it to setup the struct dirent. - * Be sure to always nul-terminate the d_name and then copy the string - * from our buffer. Note that this function assumes the full string has - * been reassembled in the buffer. If it's called before all substrings - * have been written via mbnambuf_write(), the result will be incorrect. - */ -char * -mbnambuf_flush(struct dirent *dp) -{ - - if (nambuf_len > sizeof(dp->d_name) - 1) { - mbnambuf_init(); - return (NULL); - } - bcopy(nambuf_ptr, dp->d_name, nambuf_len); - dp->d_name[nambuf_len] = '\0'; - dp->d_namlen = nambuf_len; - - mbnambuf_init(); - return (dp->d_name); -} diff -ruN msdosfs.orig/msdosfs_lookup.c msdosfs/msdosfs_lookup.c --- msdosfs.orig/msdosfs_lookup.c 2007-08-07 10:25:56.000000000 +0800 +++ msdosfs/msdosfs_lookup.c 2007-08-30 16:44:50.000000000 +0800 @@ -54,6 +54,7 @@ #include <sys/mount.h> #include <sys/namei.h> #include <sys/vnode.h> +#include <sys/dirent.h> #include <fs/msdosfs/bpb.h> #include <fs/msdosfs/direntry.h> @@ -115,7 +116,10 @@ int olddos = 1; #ifdef MSDOSFS_DEBUG - printf("msdosfs_lookup(): looking for %s\n", cnp->cn_nameptr); + printf("msdosfs_lookup(): looking for 0x"); + for (char *c = cnp->cn_nameptr; *c; ++c) + printf("%02x", (unsigned char)*c); + printf("\n"); #endif dp = VTODE(vdp); pmp = dp->de_pmp; @@ -177,15 +181,18 @@ slotcount = 0; #ifdef MSDOSFS_DEBUG - printf("msdosfs_lookup(): dos version of filename %s, length %ld\n", - dosfilename, cnp->cn_namelen); + printf("msdosfs_lookup(): dos version of filename 0x"); + for (char *c = dosfilename; *c; ++c) + printf("%02x", (unsigned char)*c); + printf(" , length %ld\n", cnp->cn_namelen); #endif /* * Search the directory pointed at by vdp for the name pointed at * by cnp->cn_nameptr. */ tdp = NULL; - mbnambuf_init(); + struct dirent namebuf; + namebuf.d_namlen = 0; /* * The outer loop ranges over the clusters that make up the * directory. Note that the root directory is different from all @@ -225,7 +232,7 @@ * Drop memory of previous long matches */ chksum = -1; - mbnambuf_init(); + namebuf.d_namlen = 0; if (slotcount < wincnt) { slotcount++; @@ -251,6 +258,7 @@ continue; chksum = win2unixfn((struct winentry *)dep, + &namebuf, chksum, pmp); continue; @@ -258,9 +266,11 @@ chksum = winChkName((const u_char *)cnp->cn_nameptr, unlen, + &namebuf, chksum, pmp); if (chksum == -2) { + namebuf.d_namlen = 0; chksum = -1; continue; } diff -ruN msdosfs.orig/msdosfs_vnops.c msdosfs/msdosfs_vnops.c --- msdosfs.orig/msdosfs_vnops.c 2007-08-07 18:35:27.000000000 +0800 +++ msdosfs/msdosfs_vnops.c 2007-08-30 15:15:45.000000000 +0800 @@ -1629,7 +1629,8 @@ } } - mbnambuf_init(); + dirbuf.d_namlen = 0; + off = offset; while (uio->uio_resid > 0) { lbn = de_cluster(pmp, offset - bias); @@ -1676,7 +1677,7 @@ */ if (dentp->deName[0] == SLOT_DELETED) { chksum = -1; - mbnambuf_init(); + dirbuf.d_namlen = 0; continue; } @@ -1686,7 +1687,7 @@ if (dentp->deAttributes == ATTR_WIN95) { if (pmp->pm_flags & MSDOSFSMNT_SHORTNAME) continue; - chksum = win2unixfn((struct winentry *)dentp, + chksum = win2unixfn((struct winentry *)dentp, &dirbuf, chksum, pmp); continue; } @@ -1696,7 +1697,7 @@ */ if (dentp->deAttributes & ATTR_VOLUME) { chksum = -1; - mbnambuf_init(); + dirbuf.d_namlen = 0; continue; } /* @@ -1738,9 +1739,8 @@ ((pmp->pm_flags & MSDOSFSMNT_SHORTNAME) ? (LCASE_BASE | LCASE_EXT) : 0), pmp); - mbnambuf_init(); - } else - mbnambuf_flush(&dirbuf); + } + chksum = -1; dirbuf.d_reclen = GENERIC_DIRSIZ(&dirbuf); if (uio->uio_resid < dirbuf.d_reclen) { @@ -1760,6 +1760,8 @@ } } off = offset + sizeof(struct direntry); + + dirbuf.d_namlen = 0; } brelse(bp); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a7033710708300835n5ac95063o6331ede5fcc80662>