kern/42573: DIRBLKSIZ in <ufs/ufs/dir.h> should not be DEV_BSIZE constant

[tsutsui%ceres.dti.ne.jp@localhost]

>Number:         42573
>Category:       kern
>Synopsis:       DIRBLKSIZ in <ufs/ufs/dir.h> should not be DEV_BSIZE constant
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Jan 03 16:55:00 +0000 2010
>Originator:     Izumi Tsutsui
>Release:        NetBSD 5.99.23
>Organization:
>Environment:
System: NetBSD  5.99.23 updated 20100103
Architecture: all
Machine: all

>Description:
DIRBLKSIZ macro is described in <ufs/ufs/dir.h> as following:
---
 * A directory consists of some number of blocks of DIRBLKSIZ
 * bytes, where DIRBLKSIZ is chosen such that it can be transferred
 * to disk in a single atomic operation (e.g. 512 bytes on most machines).
---
and it's defined using DEV_BSIZE (i.e. currently 512):
---
#undef  DIRBLKSIZ
#define DIRBLKSIZ       DEV_BSIZE
---
But "size that can be transferred to disk in a single atomic oparation"
should not be a compile-time constant but it should be determined by
a hardware sector size stored in a certain appropriate file system
specific structure.

>How-To-Repeat:
Code inspection.
You can see it by "grep DIRBLKSIZ src/sys/ufs/*/*.[ch]" etc.

>Fix:
src/sys/ufs/ffs/ffs_vfsops.c:
 We can calculate a hardware sector size (disk block size) from
 fs->fs_fsize and fs->fsbtodb.

src/sys/ufs/lfs/lfs_vfsops.c:
 We can get a hardware sector size from dlfs->fs_fsize and dlfs->fsbtodb?
 (I'm not familiar with lfs structures)

src/sys/ufs/ufs/ufs_extattr.c:
 I'm not sure how we can get the value dynamically there.

src/sys/ufs/ufs/ufs_vnops.c:
src/sys/ufs/ufs/ufs_wapbl.c:
 I'm not sure how we can get the value there.
 We also have to prepare "struct dirtamplate mastertemplate" dynamically.

src/sbin/newfs/mkfs.c:
 The necessary sector size is specified by -S option.
 We also have to prepare struct dirent for lost+found dir dynamically. 

src/sbin/usr.sbin/makefs/ffs.c:
 The necessary sector size is specified by -S option.

(have not checked other sources)

Here is a patch for makefs(8) (using DEV_BSIZE there causes fatal errors
on cross build environment on systems DEV_BSIZE != 512 like Cygwin):

Index: ffs.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/makefs/ffs.c,v
retrieving revision 1.44
diff -u -r1.44 ffs.c
--- ffs.c       28 Apr 2009 22:49:26 -0000      1.44
+++ ffs.c       3 Jan 2010 16:46:40 -0000
@@ -134,8 +134,10 @@
 
 static int     ffs_create_image(const char *, fsinfo_t *);
 static void    ffs_dump_fsinfo(fsinfo_t *);
-static void    ffs_dump_dirbuf(dirbuf_t *, const char *, int);
-static void    ffs_make_dirbuf(dirbuf_t *, const char *, fsnode *, int);
+static void    ffs_dump_dirbuf(dirbuf_t *, const char *,
+                               const fsinfo_t *);
+static void    ffs_make_dirbuf(dirbuf_t *, const char *, fsnode *,
+                               const fsinfo_t *);
 static int     ffs_populate_dir(const char *, fsnode *, fsinfo_t *);
 static void    ffs_size_dir(fsnode *, fsinfo_t *);
 static void    ffs_validate(const char *, fsnode *, fsinfo_t *);
@@ -567,8 +569,9 @@
        if (debug & DEBUG_FS_SIZE_DIR_ADD_DIRENT)                       \
                printf("ADDDIRENT: was: %s (%d) this %d cur %d\n",      \
                    e, tmpdir.d_namlen, this, curdirsize);              \
-       if (this + curdirsize > roundup(curdirsize, DIRBLKSIZ))         \
-               curdirsize = roundup(curdirsize, DIRBLKSIZ);            \
+       if (this + curdirsize >                                         \
+           roundup(curdirsize, fsopts->sectorsize))                    \
+               curdirsize = roundup(curdirsize, fsopts->sectorsize);   \
        curdirsize += this;                                             \
        if (debug & DEBUG_FS_SIZE_DIR_ADD_DIRENT)                       \
                printf("ADDDIRENT: now: %s (%d) this %d cur %d\n",      \
@@ -752,11 +755,11 @@
                                fsopts->curinode++;
                        }
                }
-               ffs_make_dirbuf(&dirbuf, cur->name, cur, fsopts->needswap);
+               ffs_make_dirbuf(&dirbuf, cur->name, cur, fsopts);
                if (cur == root) {              /* we're at "."; add ".." */
                        ffs_make_dirbuf(&dirbuf, "..",
                            cur->parent == NULL ? cur : cur->parent->first,
-                           fsopts->needswap);
+                           fsopts);
                        root->inode->nlink++;   /* count my parent's link */
                } else if (cur->child != NULL)
                        root->inode->nlink++;   /* count my child's link */
@@ -769,7 +772,7 @@
                 */
        }
        if (debug & DEBUG_FS_POPULATE_DIRBUF)
-               ffs_dump_dirbuf(&dirbuf, dir, fsopts->needswap);
+               ffs_dump_dirbuf(&dirbuf, dir, fsopts);
 
                /*
                 * pass 2: write out dirbuf, then non-directories at this level
@@ -955,14 +958,16 @@
 
 
 static void
-ffs_dump_dirbuf(dirbuf_t *dbuf, const char *dir, int needswap)
+ffs_dump_dirbuf(dirbuf_t *dbuf, const char *dir, const fsinfo_t *fsopts)
 {
        doff_t          i;
        struct direct   *de;
+       int             needswap;
        uint16_t        reclen;
 
        assert (dbuf != NULL);
        assert (dir != NULL);
+       needswap = fsopts->needswap;
        printf("ffs_dump_dirbuf: dir %s size %d cur %d\n",
            dir, dbuf->size, dbuf->cur);
 
@@ -980,15 +985,18 @@
 }
 
 static void
-ffs_make_dirbuf(dirbuf_t *dbuf, const char *name, fsnode *node, int needswap)
+ffs_make_dirbuf(dirbuf_t *dbuf, const char *name, fsnode *node,
+    const fsinfo_t *fsopts)
 {
        struct direct   de, *dp;
+       int             needswap;
        uint16_t        llen, reclen;
        u_char          *newbuf;
 
        assert (dbuf != NULL);
        assert (name != NULL);
        assert (node != NULL);
+       needswap = fsopts->needswap;
                                        /* create direct entry */
        (void)memset(&de, 0, sizeof(de));
        de.d_fileno = ufs_rw32(node->inode->ino, needswap);
@@ -1011,16 +1019,19 @@
                    ufs_rw32(de.d_fileno, needswap), de.d_type, reclen,
                    de.d_namlen, de.d_name);
 
-       if (reclen + dbuf->cur + llen > roundup(dbuf->size, DIRBLKSIZ)) {
+       if (reclen + dbuf->cur + llen >
+           roundup(dbuf->size, fsopts->sectorsize)) {
                if (debug & DEBUG_FS_MAKE_DIRBUF)
                        printf("ffs_make_dirbuf: growing buf to %d\n",
-                           dbuf->size + DIRBLKSIZ);
-               if ((newbuf = realloc(dbuf->buf, dbuf->size + DIRBLKSIZ)) == 
NULL)
+                           dbuf->size + fsopts->sectorsize);
+               if ((newbuf = realloc(dbuf->buf,
+                   dbuf->size + fsopts->sectorsize)) == NULL)
                        err(1, "Allocating memory for directory buffer");
                dbuf->buf = newbuf;
-               dbuf->size += DIRBLKSIZ;
-               memset(dbuf->buf + dbuf->size - DIRBLKSIZ, 0, DIRBLKSIZ);
-               dbuf->cur = dbuf->size - DIRBLKSIZ;
+               dbuf->size += fsopts->sectorsize;
+               memset(dbuf->buf + dbuf->size - fsopts->sectorsize, 0,
+                   fsopts->sectorsize);
+               dbuf->cur = dbuf->size - fsopts->sectorsize;
        } else if (dp) {                        /* shrink end of previous */
                dp->d_reclen = ufs_rw16(llen,needswap);
                dbuf->cur += llen;

---