Project

General

Profile

Actions

Bug #13249

closed

wasteful entry allocation in dnlc

Added by Mateusz Guzik about 1 year ago. Updated 13 days ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
kernel
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
Gerrit CR:

Description

The namecache entry has the following layout:

> ::print -a ncache_t
0 {
    0 hash_next 
    8 hash_prev 
    10 vp 
    18 dp 
    20 hash 
    24 namlen 
    25 name 
}

which implies:

> ::sizeof ncache_t
sizeof (ncache_t) = 0x28

but then dnlc_get:

        ncp = kmem_alloc(sizeof (ncache_t) + namlen, KM_NOSLEEP);

Requests 3 bytes more than expected. Instead the code should be using offsetof(ncache_t, name) here and in dnlc_free (and perhaps size computation could be deduped).

Actions #1

Updated by Mateusz Guzik about 1 year ago

Something of this sort should take care of it (not even compile tested):

diff --git a/usr/src/uts/common/fs/dnlc.c b/usr/src/uts/common/fs/dnlc.c
index 102375dedd..bf044082cf 100644
--- a/usr/src/uts/common/fs/dnlc.c
+++ b/usr/src/uts/common/fs/dnlc.c
@@ -256,7 +256,7 @@ vnode_t negative_cache_vnode;
  */
 #define        dnlc_free(ncp) \
 { \
-       kmem_free((ncp), sizeof (ncache_t) + (ncp)->namlen); \
+       kmem_free((ncp), offsetof(ncache_t, name) + (ncp)->namlen); \
        atomic_dec_32(&dnlc_nentries); \
 }

@@ -977,7 +977,7 @@ dnlc_get(uchar_t namlen)
                dnlc_max_nentries_cnt++; /* keep a statistic */
                return (NULL);
        }
-       ncp = kmem_alloc(sizeof (ncache_t) + namlen, KM_NOSLEEP);
+       ncp = kmem_alloc(offsetof(ncache_t, name) + namlen, KM_NOSLEEP);
        if (ncp == NULL) {
                return (NULL);
        }

Actions #2

Updated by Electric Monk about 1 year ago

  • Gerrit CR set to 1013
Actions #3

Updated by Toomas Soome about 1 year ago

  • Category set to kernel
  • Assignee set to Toomas Soome
  • % Done changed from 0 to 80
> ::print -ath ncache_t
0 ncache_t {
    0 struct ncache *hash_next 
    8 struct ncache *hash_prev 
    10 struct vnode *vp 
    18 struct vnode *dp 
    20 int hash 
    24 uchar_t namlen 
    25 char [0] name 
    25 unsigned <<HOLE>> :24 
}

> ::print -ath dcentry_t
0 dcentry_t {
    0 uint64_t de_handle 
    8 struct dcentry *de_next 
    10 int de_hash 
    14 uchar_t de_namelen 
    15 char [0] de_name 
    15 unsigned <<HOLE>> :24 
}

Testing done: build/install/boot.

Actions #4

Updated by Electric Monk 13 days ago

  • Status changed from New to Closed
  • % Done changed from 80 to 100

git commit e7b9901ea28b51d04ea82ac040c27457112687ec

commit  e7b9901ea28b51d04ea82ac040c27457112687ec
Author: Toomas Soome <tsoome@me.com>
Date:   2022-01-04T23:01:48.000Z

    13249 wasteful entry allocation in dnlc
    Reviewed by: Klaus Ziegler <klausz@haus-gisela.de>
    Approved by: Robert Mustacchi <rm@fingolfin.org>

Actions #5

Updated by Marcel Telka 13 days ago

It looks like this fix removes the terminating nul from both ncache_t:name and dcentry_t:de_name. How will things like dnlcdump() (mdb) work now?

Actions #6

Updated by Toomas Soome 13 days ago

Marcel Telka wrote in #note-5:

It looks like this fix removes the terminating nul from both ncache_t:name and dcentry_t:de_name. How will things like dnlcdump() (mdb) work now?

It seems to be just fine; this is because we set up dnlc data in mdb with dnlc_load(), which does allocate entry with MDB_DNLC_NCACHE_SZ() macro, which does sizeof (struct) + namelen and the name entry is properly terminated with 0. So, yep, the support code does seem to correctly take account that dnlc struct is meant to be struct + variable sized string, just the dnlc buildup itself was messing things up a bit.

Actions #7

Updated by Marcel Telka 13 days ago

Toomas Soome wrote in #note-6:

Marcel Telka wrote in #note-5:

It looks like this fix removes the terminating nul from both ncache_t:name and dcentry_t:de_name. How will things like dnlcdump() (mdb) work now?

It seems to be just fine; this is because we set up dnlc data in mdb with dnlc_load(), which does allocate entry with MDB_DNLC_NCACHE_SZ() macro, which does sizeof (struct) + namelen and the name entry is properly terminated with 0. So, yep, the support code does seem to correctly take account that dnlc struct is meant to be struct + variable sized string, just the dnlc buildup itself was messing things up a bit.

You are right. Thanks for explanation and confirmation. I missed the following line in dnlc_load():

307              ncp->name[ncp->namlen] = '\0';
Actions

Also available in: Atom PDF