1603801 - [patch] Avoid dcache pollution from sdb_measureAccess()

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Eric Sandeen]

Attachment: nss-softokn-sdb_measureAccess.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

As implemented, when sdb_measureAccess() runs it creates up to 10,000 negative dcache entries (cached nonexistent filenames).

There is no advantage to leaving these particular filenames in the cache; they will never be searched again. Subsequent runs will run a new test with an intentionally different set of filenames. This can have detrimental effects on some systems; a massive negative dcache can lead to memory or performance problems.

A google search for NSS_SDB_USE_CACHE shows many bugs filed, tips, tricks etc to mitigate this issue, and it's up to the admin to get it right in some cases.

It's possible to run sdb_measureAccess() in a way that, at least on modern Linux kernels, does not leave negative dcache pollution.

If the test is run in a temporary subdirectory and the subdirectory is removed after the test, the negative dcache entries will be removed from memory as well. This is a much kinder way to treat the dcache when it is known that the negative dcache entries hold no value.

I have attached a patch to do this as a proof of concept. It works for me, and I am happy to have it reviewed or modified to meet libnss standards as needed. It has been tested with v3.44 but I have confirmed that it applies cleanly to current code.

This was motivated by Red Hat bug
Bug 1779325 - when NSS_SDB_USE_CACHE not set, after curl access https, dentry increase but never released

(we saw a user with nearly 1T of negative dcache generated primarily via this mechanism)

Thanks,
-Eric

Comment 1

[Eric Sandeen]

Note, negative dcache entries can be tracked on a modern Linux kernel via /proc/sys/fs/dentry-state

# cat /proc/sys/fs/dentry-state
690051	666440	45	0	29022	0

dentry-state
------------

From linux/include/linux/dcache.h::

  struct dentry_stat_t dentry_stat {
        int nr_dentry;
        int nr_unused;
        int age_limit;         /* age in seconds */
        int want_pages;        /* pages requested by system */
        int nr_negative;       /* # of unused negative dentries */
        int dummy;             /* Reserved for future use */
  };

Dentries are dynamically allocated and deallocated.

nr_dentry shows the total number of dentries allocated (active + unused). nr_unused shows the number of dentries that are not actively used, but are saved in the LRU list for future reuse.

Age_limit is the age in seconds after which dcache entries can be reclaimed when memory is short and want_pages is nonzero when shrink_dcache_pages() has been called and the dcache isn't pruned yet.

nr_negative shows the number of unused dentries that are also negative dentries which do not map to any files. Instead, they help speeding up rejection of non-existing files provided by the users.

Comment 2

[Robert Relyea]

Attachment: nss-softokn-sdb_measureAccess.patch

Load Patch as patch, add reviewers.

Comment 3

[Robert Relyea]

Martin, what is our current position on use of PORT_xxxx for string functions. I have usually tried to keep it, but I've noticed a few occurances of strcat rather than PORT_Strcat in the code. I've noticed a lot of occurances of strlen rather than PORT_Strlen, including in the prepatched version of this code. Similarly what about non-nspr calls for directory functions. NSPR has a PR_Rmdir, but it doesn't seem to have an equivalent of mkdtemp. Is it available on Windows?

Comment 4

[Martin Thomson [:mt:]]

I think that there are a bunch of things that we might want to discuss this general class of things.

There are a bunch of things that are widely available now, such as <stdint.h>, memcpy, strlen, and other things that secport.h was originally intended to make consistent. I note that uint32_t is in that class of things as well. This patch shows that we have size_t also, which is tricky because unconstrained use might turn into ABI compatibility issues.

What has happened is that we've seen those creep into the codebase in small ways without any conscious policy decision. I've got some vacation time now, but I'll take it up on the nss-dev list before I leave. I think we should start with small things and then work out what we might do with each in turn. It seems like secport.h has a few things we might want to make decisions about.

As for this specific patch, mkdtemp() is ideal, but it doesn't exist on Windows, so we need platform-specific code here. Now maybe Windows doesn't suffer the same dcache issues and so we can skip that invocation, but we should make that decision explicit. Also, _rmdir is prefered on Windows.

I have some other comments on the code, but I can't use splinter to review this. Can we move to Phabricator?

Comment 5

[Martin Thomson [:mt:]]

Attachment: Bug 1603801 - sdb_measureAccess: test lookups in temp subdir

When sdb_measureAccess runs, it creates up to 10,000 negative
dcache entries for the dOeSnotExist.db files. There is
no need to hang onto these negative dentries, because they
will never be searched again. Be a little kinder to the
dcache by doing this test in a temporary subdir; when that
subdir gets removed post-test, the negative dentries get
removed as well. This addresses the problem of nearly unlimited
negative dcache expansion on Linux boxes, which historically
has not been handled well by the OS.

Signed-off-by: Eric Sandeen <[e-mail]>

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 9116435 [details] [diff] [review]
nss-softokn-sdb_measureAccess.patch

Review of attachment 9116435 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing my review flag here.  I uploaded this to phabricator, but I want to be clear that I'm not working on this.  If someone else wants to commandeer this and complete it (I think that it's simple enough), then I'd be happy to review.  Right now, I don't have the time to build this and test it.

Comment 7

[Eric Sandeen]

Thanks for beating this patch into shape and getting it merged.

Comment 8

[Eric Sandeen]

It looks like the patch as ultimately committed has an error which persists in the upstream code.

These files:
nss/coreconf/config.gypi
nss/coreconf/Linux.mk

define SQL_MEASURE_USE_TEMP_DIR, but the code itself tests for SDB_MEASURE_USE_TEMP_DIR

(note SQL vs. SDB)

Comment 9

[Robert Relyea]

Yup, reopening bug.

Comment 10

[Robert Relyea]

Comment on attachment 9116435 [details] [diff] [review]
nss-softokn-sdb_measureAccess.patch

clearing review request. patch was reviewed in phabricator.

Comment 11

[Dennis Jackson]

Bob, can we close this?

Comment 12

[Robert Relyea]

Yes:
$ grep MEASURE_USE nss/coreconf/*
nss/coreconf/config.gypi: 'SDB_MEASURE_USE_TEMP_DIR', # use tmpdir for the access calls
nss/coreconf/Linux.mk:DEFINES += -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -DSDB_MEASURE_USE_TEMP_DIR