Bug #13249
closedwasteful entry allocation in dnlc
100%
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).
Updated by Mateusz Guzik over 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); }
Updated by Toomas Soome over 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.
Updated by Electric Monk 5 months 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>
Updated by Marcel Telka 5 months 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?
Updated by Toomas Soome 5 months ago
Marcel Telka wrote in #note-5:
It looks like this fix removes the terminating nul from both
ncache_t:name
anddcentry_t:de_name
. How will things likednlcdump()
(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.
Updated by Marcel Telka 5 months 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
anddcentry_t:de_name
. How will things likednlcdump()
(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';