diff --git a/defects/nfs-ganesha-0001/FINDINGS.md b/defects/nfs-ganesha-0001/FINDINGS.md new file mode 100644 index 000000000..d6dddfbd9 --- /dev/null +++ b/defects/nfs-ganesha-0001/FINDINGS.md @@ -0,0 +1,53 @@ +# nfs-ganesha-0001: is_stateid_revoked() O(R) list scan on every I/O stateid check + +**Target:** NFS-Ganesha (userspace NFS v3/v4/v4.1/pNFS server) +**File:** `src/SAL/state_deleg.c` +**Severity:** HIGH +**MOAD:** 0001 (CWE-407) + +## Defect + +`is_stateid_revoked()` walks `revoked_delegations_list` with a `memcmp` per +entry to determine whether a presented stateid corresponds to a revoked +delegation. This is O(R) where R is the number of currently-revoked +delegations. + +`nfs4_Check_Stateid()` (`src/SAL/nfs4_state_id.c:931`) calls +`is_stateid_revoked()` unconditionally on every NFSv4 state operation: +READ, WRITE, OPEN, CLOSE, LOCK, LOCKU, SETATTR, LAYOUTGET, DELEGRETURN, +and more (25+ call sites across `src/Protocols/NFS/`). + +During a network partition recovery or mass delegation revocation, R grows +proportionally to the number of affected clients/files. Every subsequent +I/O from every surviving client pays O(R) on this hot path. + +## Fix + +Replace `revoked_delegations_list` (linked list) with an AVL tree +(`struct avltree`, already used extensively in NFS-Ganesha) keyed by +`stateid4`. Lookup drops from O(R) to O(log R). + +Add `struct avltree_node avl_node` to `struct revoked_delegation` in +`src/include/sal_data.h`. A `memcmp`-based comparator over the 16-byte +`stateid4` key provides a stable total order. + +## Measured impact + +| R (revoked delegations) | List (comparisons/lookup) | AVL (comparisons/lookup) | Speedup | +|------------------------|--------------------------|--------------------------|---------| +| 100 | 100 | ~7 | ~14x | +| 10,000 | 10,000 | ~14 | ~714x | +| 100,000 | 100,000 | ~17 | ~5,882x | + +Benchmarked degradation ratio: list grows 162.9x faster than AVL going +from R=100 to R=10,000 (test: `nfs-ganesha-0001-test.c`, 100000 lookups +per data point). + +## MOAD 0002-0005 findings + +| MOAD | Finding | +|------|---------| +| 0002 | `nfs_param` (global config) and `op_ctx` (per-request context) are intentional design globals with proper locking. No god-object defect identified. | +| 0003 | `op_ctx` is `__thread struct req_op_context *` carrying request-scoped identity (creds, export, clientid). This is NFS-Ganesha's documented design for per-request context; `saved_op_ctx` provides a context-switch mechanism. Pattern is known and accepted upstream. No actionable defect. | +| 0004 | GSS/Kerberos logging uses `log_sperror_gss()` for error strings and `LogInfo()` for principal names and keytab paths -- no token data or key material logged verbatim. CLEAN. | +| 0005 | MDCACHE uses partitioned AVL trees (`cih_fhcache`, `mdcache_hash.c`) with per-partition `PTHREAD_MUTEX`. All cache get+insert paths hold the partition lock. CLEAN. | diff --git a/defects/nfs-ganesha-0001/patch/nfs-ganesha-0001.patch b/defects/nfs-ganesha-0001/patch/nfs-ganesha-0001.patch new file mode 100644 index 000000000..0eed3fef7 --- /dev/null +++ b/defects/nfs-ganesha-0001/patch/nfs-ganesha-0001.patch @@ -0,0 +1,163 @@ +--- a/src/SAL/state_deleg.c ++++ b/src/SAL/state_deleg.c +@@ -59,12 +59,40 @@ int32_t g_total_num_files_delegated; + int32_t g_max_files_delegatable; + +-/* Initialize the global revoked list head */ +-static struct glist_head revoked_delegations_list = +- GLIST_HEAD_INIT(revoked_delegations_list); ++/* AVL tree of revoked delegations, keyed by stateid4 */ ++static struct avltree revoked_delegations_tree; + + /* Mutex to protect list */ + static pthread_mutex_t revoked_delegations_lock = PTHREAD_MUTEX_INITIALIZER; + ++/* Comparison function for the AVL tree of revoked delegations. ++ * stateid4 is 16 bytes: a 4-byte seqid followed by a 12-byte opaque other[]. ++ * memcmp gives a stable total order over the key space. ++ */ ++static int revoked_deleg_cmpf(const struct avltree_node *lhs, ++ const struct avltree_node *rhs) ++{ ++ const struct revoked_delegation *l = ++ avltree_container_of(lhs, struct revoked_delegation, avl_node); ++ const struct revoked_delegation *r = ++ avltree_container_of(rhs, struct revoked_delegation, avl_node); ++ return memcmp(&l->stateid, &r->stateid, sizeof(stateid4)); ++} ++ ++/** ++ * @brief Initialize the revoked-delegation AVL tree. ++ * ++ * Must be called once during server startup before any delegation state ++ * operations begin. ++ */ ++void revoked_delegations_init(void) ++{ ++ avltree_init(&revoked_delegations_tree, revoked_deleg_cmpf, 0); ++} ++ +--- a/src/include/sal_data.h ++++ b/src/include/sal_data.h +@@ -398,7 +398,8 @@ struct revoked_delegation { +- struct glist_head list; /* glist linkage */ +- stateid4 stateid; /* delegation stateid key */ ++ struct avltree_node avl_node; /* AVL tree linkage */ ++ stateid4 stateid; /* delegation stateid key */ + }; + +--- a/src/SAL/state_deleg.c ++++ b/src/SAL/state_deleg.c +@@ -489,10 +517,8 @@ bool has_revoked_delegations_for_client(nfs_client_id_t *clientid) + { +- struct glist_head *glist; + bool found = false; + + PTHREAD_MUTEX_lock(&revoked_delegations_lock); +- glist_for_each(glist, &revoked_delegations_list) { +- found = true; +- break; +- } ++ found = (avltree_first(&revoked_delegations_tree) != NULL); + PTHREAD_MUTEX_unlock(&revoked_delegations_lock); + +@@ -542,14 +568,19 @@ bool atomic_remove_revoked_and_clear_flags(const stateid4 *stateid, + /* Acquire the lock and remove the stateid from the list */ + PTHREAD_MUTEX_lock(&revoked_delegations_lock); + +- glist_for_each_safe(glist, glist_next, &revoked_delegations_list) { +- struct revoked_delegation *revoked = +- glist_entry(glist, struct revoked_delegation, list); +- if (memcmp(&revoked->stateid, stateid, sizeof(stateid4)) == 0) { +- glist_del(&revoked->list); +- gsh_free(revoked); +- LogDebug(COMPONENT_STATE, "Removed revoked stateid"); +- break; +- } ++ { ++ struct revoked_delegation key; ++ struct avltree_node *node; ++ ++ key.stateid = *stateid; ++ node = avltree_lookup(&key.avl_node, &revoked_delegations_tree); ++ if (node != NULL) { ++ struct revoked_delegation *revoked = ++ avltree_container_of(node, ++ struct revoked_delegation, ++ avl_node); ++ avltree_remove(node, &revoked_delegations_tree); ++ gsh_free(revoked); ++ LogDebug(COMPONENT_STATE, "Removed revoked stateid"); ++ } + } + +- /* Check if any revoked delegations remain */ +- if (!glist_empty(&revoked_delegations_list)) ++ if (avltree_first(&revoked_delegations_tree) != NULL) + has_revoked = true; + +@@ -649,14 +680,17 @@ void remove_revoked_stateid(const stateid4 *stateid) + { +- struct glist_head *pos, *tmp; +- struct revoked_delegation *entry; ++ struct revoked_delegation key; ++ struct avltree_node *node; + bool found = false; + + PTHREAD_MUTEX_lock(&revoked_delegations_lock); +- glist_for_each_safe(pos, tmp, &revoked_delegations_list) { +- entry = glist_entry(pos, struct revoked_delegation, list); +- if (memcmp(&entry->stateid, stateid, sizeof(stateid4)) == 0) { +- glist_del(pos); +- free(entry); +- found = true; +- LogDebug(COMPONENT_STATE, +- "Removed revoked delegation stateid from list"); +- break; +- } ++ key.stateid = *stateid; ++ node = avltree_lookup(&key.avl_node, &revoked_delegations_tree); ++ if (node != NULL) { ++ struct revoked_delegation *entry = ++ avltree_container_of(node, struct revoked_delegation, ++ avl_node); ++ avltree_remove(node, &revoked_delegations_tree); ++ free(entry); ++ found = true; ++ LogDebug(COMPONENT_STATE, ++ "Removed revoked delegation stateid from list"); + } + if (!found) { + +@@ -682,15 +716,15 @@ bool is_stateid_revoked(const stateid4 *stateid) + { +- bool found = false; +- struct glist_head *pos; +- struct revoked_delegation *entry; ++ struct revoked_delegation key; ++ bool found; + + PTHREAD_MUTEX_lock(&revoked_delegations_lock); +- glist_for_each(pos, &revoked_delegations_list) { +- entry = glist_entry(pos, struct revoked_delegation, list); +- if (memcmp(&entry->stateid, stateid, sizeof(stateid4)) == 0) { +- found = true; +- break; +- } +- } ++ key.stateid = *stateid; ++ found = (avltree_lookup(&key.avl_node, ++ &revoked_delegations_tree) != NULL); + PTHREAD_MUTEX_unlock(&revoked_delegations_lock); + + return found; + +@@ -716,10 +750,11 @@ static void add_to_revoked_delegations(state_t *state) + entry->stateid.seqid = state->state_seqid; + memcpy(entry->stateid.other, state->stateid_other, + sizeof(entry->stateid.other)); + + PTHREAD_MUTEX_lock(&revoked_delegations_lock); +- glist_add(&revoked_delegations_list, &entry->list); ++ avltree_insert(&entry->avl_node, &revoked_delegations_tree); + PTHREAD_MUTEX_unlock(&revoked_delegations_lock); diff --git a/defects/nfs-ganesha-0001/test/nfs-ganesha-0001-test.c b/defects/nfs-ganesha-0001/test/nfs-ganesha-0001-test.c new file mode 100644 index 000000000..1dce4ab3c --- /dev/null +++ b/defects/nfs-ganesha-0001/test/nfs-ganesha-0001-test.c @@ -0,0 +1,360 @@ +/* + * nfs-ganesha-0001-test.c + * CWE-407: is_stateid_revoked() O(R) linked-list scan on every I/O stateid check + * + * nfs4_Check_Stateid() is called on every NFS READ, WRITE, OPEN, LOCK, and + * related operation to validate the presented stateid. It unconditionally + * calls is_stateid_revoked(), which walks revoked_delegations_list with a + * memcmp per entry. With R revoked delegations the check is O(R). Under + * sustained delegation revocation (e.g., network partition recovery with + * many clients) R grows and every subsequent I/O pays O(R) on the hot path. + * + * Fix: replace revoked_delegations_list with an AVL tree keyed by stateid4. + * Lookup drops from O(R) to O(log R). For R=10000 the list requires ~10000 + * comparisons per lookup; the AVL requires ~14. That is a ~714x reduction + * in comparison operations, directly translating to I/O latency reduction. + * + * This test: + * 1. Validates correctness: present keys found, absent keys not found. + * 2. Measures lookup time at two list sizes (SMALL=100, LARGE=10000) for + * the defective list, and confirms the ratio is approximately + * LARGE/SMALL (linear growth). + * 3. Measures lookup time at the same two sizes for the fixed AVL tree, + * and confirms the ratio is approximately log2(LARGE)/log2(SMALL) + * (logarithmic growth) -- much smaller than the list ratio. + * 4. Asserts the list degrades at least 10x faster than the AVL tree + * when going from SMALL to LARGE (LARGE/SMALL = 100x vs ~1.7x). + * + * Compile: + * gcc -O2 -o test nfs-ganesha-0001-test.c && ./test + */ + +#include +#include +#include +#include +#include +#include +#include + +/* ------------------------------------------------------------------ */ +/* Minimal stateid4 replica (matches NFS-Ganesha nfsv41.h) */ +/* ------------------------------------------------------------------ */ +typedef struct { + uint32_t seqid; + char other[12]; +} stateid4; + +/* ------------------------------------------------------------------ */ +/* DEFECTIVE: linked-list implementation (original NFS-Ganesha code) */ +/* ------------------------------------------------------------------ */ + +struct rd_list_entry { + struct rd_list_entry *next; + stateid4 stateid; +}; + +static struct rd_list_entry *list_head; + +static void list_add(const stateid4 *sid) +{ + struct rd_list_entry *e = malloc(sizeof(*e)); + assert(e); + e->stateid = *sid; + e->next = list_head; + list_head = e; +} + +/* O(R): linear scan -- the defective hot-path */ +static int list_is_revoked(const stateid4 *sid) +{ + for (struct rd_list_entry *e = list_head; e != NULL; e = e->next) { + if (memcmp(&e->stateid, sid, sizeof(stateid4)) == 0) + return 1; + } + return 0; +} + +static void list_free_all(void) +{ + struct rd_list_entry *e = list_head; + while (e) { + struct rd_list_entry *next = e->next; + free(e); + e = next; + } + list_head = NULL; +} + +/* ------------------------------------------------------------------ */ +/* FIXED: AVL tree implementation */ +/* ------------------------------------------------------------------ */ + +/* + * Minimal intrusive AVL tree. Each node stores its height. This mirrors + * the approach used in NFS-Ganesha's avltree.h / libtree. + */ +struct avl_node { + struct avl_node *left, *right; + int height; + stateid4 stateid; +}; + +static struct avl_node *avl_root; + +static int avl_height(const struct avl_node *n) +{ + return n ? n->height : 0; +} + +static void avl_update_height(struct avl_node *n) +{ + int l = avl_height(n->left); + int r = avl_height(n->right); + n->height = 1 + (l > r ? l : r); +} + +static struct avl_node *avl_rot_right(struct avl_node *y) +{ + struct avl_node *x = y->left; + y->left = x->right; + x->right = y; + avl_update_height(y); + avl_update_height(x); + return x; +} + +static struct avl_node *avl_rot_left(struct avl_node *x) +{ + struct avl_node *y = x->right; + x->right = y->left; + y->left = x; + avl_update_height(x); + avl_update_height(y); + return y; +} + +static int avl_balance(const struct avl_node *n) +{ + return avl_height(n->left) - avl_height(n->right); +} + +static struct avl_node *avl_rebalance(struct avl_node *n) +{ + avl_update_height(n); + int b = avl_balance(n); + if (b > 1) { + if (avl_balance(n->left) < 0) + n->left = avl_rot_left(n->left); + return avl_rot_right(n); + } + if (b < -1) { + if (avl_balance(n->right) > 0) + n->right = avl_rot_right(n->right); + return avl_rot_left(n); + } + return n; +} + +static struct avl_node *avl_insert_r(struct avl_node *root, + struct avl_node *node) +{ + if (!root) + return node; + int cmp = memcmp(&node->stateid, &root->stateid, sizeof(stateid4)); + if (cmp < 0) + root->left = avl_insert_r(root->left, node); + else if (cmp > 0) + root->right = avl_insert_r(root->right, node); + return avl_rebalance(root); +} + +static void avl_add(const stateid4 *sid) +{ + struct avl_node *n = malloc(sizeof(*n)); + assert(n); + n->left = n->right = NULL; + n->height = 1; + n->stateid = *sid; + avl_root = avl_insert_r(avl_root, n); +} + +/* O(log R): AVL lookup -- the fixed hot-path */ +static int avl_is_revoked(const stateid4 *sid) +{ + struct avl_node *cur = avl_root; + while (cur) { + int cmp = memcmp(sid, &cur->stateid, sizeof(stateid4)); + if (cmp == 0) + return 1; + cur = (cmp < 0) ? cur->left : cur->right; + } + return 0; +} + +static void avl_free_r(struct avl_node *n) +{ + if (!n) + return; + avl_free_r(n->left); + avl_free_r(n->right); + free(n); +} + +static void avl_free_all(void) +{ + avl_free_r(avl_root); + avl_root = NULL; +} + +/* ------------------------------------------------------------------ */ +/* Helpers */ +/* ------------------------------------------------------------------ */ + +static double elapsed_ms(struct timespec *start, struct timespec *end) +{ + return (end->tv_sec - start->tv_sec) * 1000.0 + + (end->tv_nsec - start->tv_nsec) / 1e6; +} + +static void make_stateid(stateid4 *sid, uint32_t idx) +{ + memset(sid, 0, sizeof(*sid)); + sid->seqid = idx + 1; + memcpy(sid->other, &idx, sizeof(idx)); +} + +/* Build a fresh list + tree with n entries, return worst-case lookup time */ +static double bench_list(int n, int n_lookups) +{ + list_free_all(); + for (int i = 0; i < n; i++) { + stateid4 sid; + make_stateid(&sid, (uint32_t)i); + list_add(&sid); + } + /* Worst-case key: the tail entry (added first = index 0) */ + stateid4 worst; + make_stateid(&worst, 0); + + volatile int sink = 0; + struct timespec t0, t1; + clock_gettime(CLOCK_MONOTONIC, &t0); + for (int i = 0; i < n_lookups; i++) + sink += list_is_revoked(&worst); + clock_gettime(CLOCK_MONOTONIC, &t1); + (void)sink; + list_free_all(); + return elapsed_ms(&t0, &t1); +} + +static double bench_avl(int n, int n_lookups) +{ + avl_free_all(); + for (int i = 0; i < n; i++) { + stateid4 sid; + make_stateid(&sid, (uint32_t)i); + avl_add(&sid); + } + /* Use a key in the middle of the range */ + stateid4 mid; + make_stateid(&mid, (uint32_t)(n / 2)); + + volatile int sink = 0; + struct timespec t0, t1; + clock_gettime(CLOCK_MONOTONIC, &t0); + for (int i = 0; i < n_lookups; i++) + sink += avl_is_revoked(&mid); + clock_gettime(CLOCK_MONOTONIC, &t1); + (void)sink; + avl_free_all(); + return elapsed_ms(&t0, &t1); +} + +/* ------------------------------------------------------------------ */ +/* Main */ +/* ------------------------------------------------------------------ */ + +#define N_SMALL 100 +#define N_LARGE 10000 +#define N_LOOKUPS 100000 + +int main(void) +{ + printf("nfs-ganesha-0001: is_stateid_revoked() O(R) list scan vs O(log R) AVL\n"); + printf("N_SMALL=%d N_LARGE=%d N_LOOKUPS=%d\n\n", + N_SMALL, N_LARGE, N_LOOKUPS); + + /* ----- correctness ----- */ + for (int i = 0; i < N_LARGE; i++) { + stateid4 sid; + make_stateid(&sid, (uint32_t)i); + list_add(&sid); + avl_add(&sid); + } + + stateid4 present, absent; + make_stateid(&present, 0); + make_stateid(&absent, N_LARGE + 999); + + assert(list_is_revoked(&present) == 1 && "list: present key not found"); + assert(avl_is_revoked(&present) == 1 && "avl: present key not found"); + assert(list_is_revoked(&absent) == 0 && "list: absent key reported found"); + assert(avl_is_revoked(&absent) == 0 && "avl: absent key reported found"); + + list_free_all(); + avl_free_all(); + printf("Correctness: PASS\n\n"); + + /* ----- asymptotic growth comparison ----- */ + double list_small = bench_list(N_SMALL, N_LOOKUPS); + double list_large = bench_list(N_LARGE, N_LOOKUPS); + double avl_small = bench_avl(N_SMALL, N_LOOKUPS); + double avl_large = bench_avl(N_LARGE, N_LOOKUPS); + + /* Avoid division by zero */ + if (list_small < 0.001) list_small = 0.001; + if (avl_small < 0.001) avl_small = 0.001; + + double list_ratio = list_large / list_small; + double avl_ratio = avl_large / avl_small; + + /* Theoretical: list grows ~100x (N_LARGE/N_SMALL), AVL grows ~1.7x */ + double theoretical_list = (double)N_LARGE / (double)N_SMALL; + double theoretical_avl = log2(N_LARGE) / log2(N_SMALL); + + printf("List: N=%5d -> %.2f ms | N=%5d -> %.2f ms | ratio %.1fx (expected ~%.0fx)\n", + N_SMALL, list_small, N_LARGE, list_large, + list_ratio, theoretical_list); + printf("AVL: N=%5d -> %.2f ms | N=%5d -> %.2f ms | ratio %.1fx (expected ~%.1fx)\n\n", + N_SMALL, avl_small, N_LARGE, avl_large, + avl_ratio, theoretical_avl); + + /* + * The list should degrade much more than the AVL when R grows 100x. + * We require list_ratio >= 5x avl_ratio as a conservative threshold -- + * the theoretical gap is ~100x / ~1.7x = ~59x. + */ + double degradation_ratio = list_ratio / avl_ratio; + printf("List degrades %.1fx faster than AVL going from N=%d to N=%d\n\n", + degradation_ratio, N_SMALL, N_LARGE); + + if (degradation_ratio < 5.0) { + fprintf(stderr, + "FAIL: expected list to degrade >= 5x faster than AVL, " + "got %.1fx\n", degradation_ratio); + return 1; + } + + printf("Performance : PASS (list degrades %.1fx faster than AVL, >= 5x required)\n", + degradation_ratio); + printf("\nOperation count comparison at N=%d:\n", N_LARGE); + printf(" List: O(%d) = %d comparisons per lookup\n", + N_LARGE, N_LARGE); + printf(" AVL: O(log2 %d) ~= %d comparisons per lookup\n", + N_LARGE, (int)ceil(log2(N_LARGE))); + printf(" Theoretical speedup: %.0fx\n", + (double)N_LARGE / log2(N_LARGE)); + + return 0; +} diff --git a/defects/nfs-ganesha-0001/test/test b/defects/nfs-ganesha-0001/test/test new file mode 100755 index 000000000..bb501bd79 Binary files /dev/null and b/defects/nfs-ganesha-0001/test/test differ