- From fe15550b67e1d51c326f51d06c33beb247f0d9b3 Mon Sep 17 00:00:00 2001
- From: Roland Mainz <roland.mainz@nrubsig.org>
- Date: Thu, 26 Jun 2025 18:39:52 +0200
- Subject: [PATCH 1/2] sys: Fix kernel ACL cache
- Fix kernel ACL cache.
- This cache has only the lifetime of an open file handle, it's intended
- to avoid server roundtrips in case a program queries the ACL many times
- for the same file handle.
- Reported-by: Dan Shelton <dan.f.shelton@gmail.com>
- Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com>
- ---
- sys/nfs41sys_acl.c | 88 ++++++++++++++++++++++++++++++++++------------
- 1 file changed, 65 insertions(+), 23 deletions(-)
- diff --git a/sys/nfs41sys_acl.c b/sys/nfs41sys_acl.c
- index c468433..fce29f5 100644
- --- a/sys/nfs41sys_acl.c
- +++ b/sys/nfs41sys_acl.c
- @@ -213,6 +213,8 @@ NTSTATUS nfs41_QuerySecurityInformation(
- NFS41GetNetRootExtension(SrvOpen->pVNetRoot->pNetRoot);
- SECURITY_INFORMATION info_class =
- RxContext->CurrentIrpSp->Parameters.QuerySecurity.SecurityInformation;
- + ULONG querysecuritylength =
- + RxContext->CurrentIrpSp->Parameters.QuerySecurity.Length;
- #ifdef ENABLE_TIMINGS
- LARGE_INTEGER t1, t2;
- t1 = KeQueryPerformanceCounter(NULL);
- @@ -235,7 +237,19 @@ NTSTATUS nfs41_QuerySecurityInformation(
- (long long)current_time.QuadPart,
- (long long)nfs41_fobx->time.QuadPart);
- #endif
- - if (current_time.QuadPart - nfs41_fobx->time.QuadPart <= 20*1000) {
- + if ((current_time.QuadPart - nfs41_fobx->time.QuadPart) <= (10*1000)) {
- + if (querysecuritylength < nfs41_fobx->acl_len) {
- + status = STATUS_BUFFER_OVERFLOW;
- + RxContext->InformationToReturn = nfs41_fobx->acl_len;
- +
- + DbgP("nfs41_QuerySecurityInformation: "
- + "STATUS_BUFFER_OVERFLOW for cached entry, "
- + "got %lu, need %lu\n",
- + (unsigned long)querysecuritylength,
- + (unsigned long)nfs41_fobx->acl_len);
- + goto out;
- + }
- +
- PSECURITY_DESCRIPTOR sec_desc = (PSECURITY_DESCRIPTOR)
- RxContext->CurrentIrp->UserBuffer;
- RtlCopyMemory(sec_desc, nfs41_fobx->acl, nfs41_fobx->acl_len);
- @@ -246,12 +260,17 @@ NTSTATUS nfs41_QuerySecurityInformation(
- InterlockedIncrement(&getacl.sops);
- InterlockedAdd64(&getacl.size, nfs41_fobx->acl_len);
- #endif
- - } else status = 1;
- - RxFreePool(nfs41_fobx->acl);
- - nfs41_fobx->acl = NULL;
- - nfs41_fobx->acl_len = 0;
- - if (!status)
- +
- + DbgP("nfs41_QuerySecurityInformation: using cached ACL info\n");
- goto out;
- + } else {
- + if (nfs41_fobx->acl) {
- + RxFreePool(nfs41_fobx->acl);
- + nfs41_fobx->acl = NULL;
- + nfs41_fobx->acl_len = 0;
- + }
- + DbgP("nfs41_QuerySecurityInformation: cached ACL info invalidated\n");
- + }
- }
- status = nfs41_UpcallCreate(NFS41_SYSOP_ACL_QUERY, &nfs41_fobx->sec_ctx,
- @@ -263,7 +282,7 @@ NTSTATUS nfs41_QuerySecurityInformation(
- /* we can't provide RxContext->CurrentIrp->UserBuffer to the upcall thread
- * because it becomes an invalid pointer with that execution context
- */
- - entry->buf_len = RxContext->CurrentIrpSp->Parameters.QuerySecurity.Length;
- + entry->buf_len = querysecuritylength;
- status = nfs41_UpcallWaitForReply(entry, pVNetRootContext->timeout);
- if (status) {
- @@ -273,36 +292,52 @@ NTSTATUS nfs41_QuerySecurityInformation(
- }
- if (entry->status == STATUS_BUFFER_TOO_SMALL) {
- -#ifdef DEBUG_ACL_QUERY
- - DbgP("nfs41_QuerySecurityInformation: provided buffer size=%d but we "
- - "need %lu\n",
- - RxContext->CurrentIrpSp->Parameters.QuerySecurity.Length,
- - entry->buf_len);
- -#endif
- + DbgP("nfs41_QuerySecurityInformation: "
- + "STATUS_BUFFER_OVERFLOW for entry, "
- + "got %lu, need %lu\n",
- + (unsigned long)querysecuritylength,
- + (unsigned long)entry->buf_len);
- status = STATUS_BUFFER_OVERFLOW;
- RxContext->InformationToReturn = entry->buf_len;
- - /* Save ACL buffer */
- + if (entry->buf) {
- + RxFreePool(entry->buf);
- + entry->buf = NULL;
- + }
- + } else if (entry->status == STATUS_SUCCESS) {
- + /*
- + * Free previous ACL data. This can happen if two concurrent
- + * requests are executed for the same file
- + */
- + if (nfs41_fobx->acl) {
- + RxFreePool(nfs41_fobx->acl);
- + nfs41_fobx->acl = NULL;
- + nfs41_fobx->acl_len = 0;
- + }
- +
- nfs41_fobx->acl = entry->buf;
- nfs41_fobx->acl_len = entry->buf_len;
- + entry->buf = NULL;
- KeQuerySystemTime(&nfs41_fobx->time);
- - } else if (entry->status == STATUS_SUCCESS) {
- +
- PSECURITY_DESCRIPTOR sec_desc = (PSECURITY_DESCRIPTOR)
- RxContext->CurrentIrp->UserBuffer;
- - RtlCopyMemory(sec_desc, entry->buf, entry->buf_len);
- + RtlCopyMemory(sec_desc, nfs41_fobx->acl, nfs41_fobx->acl_len);
- + RxContext->IoStatusBlock.Information =
- + RxContext->InformationToReturn = nfs41_fobx->acl_len;
- + RxContext->IoStatusBlock.Status = status = STATUS_SUCCESS;
- +
- #ifdef ENABLE_TIMINGS
- InterlockedIncrement(&getacl.sops);
- InterlockedAdd64(&getacl.size, entry->u.Acl.buf_len);
- #endif
- - RxFreePool(entry->buf);
- - entry->buf = NULL;
- - nfs41_fobx->acl = NULL;
- - nfs41_fobx->acl_len = 0;
- - RxContext->IoStatusBlock.Information = RxContext->InformationToReturn =
- - entry->buf_len;
- - RxContext->IoStatusBlock.Status = status = STATUS_SUCCESS;
- } else {
- status = map_query_acl_error(entry->status);
- +
- + if (entry->buf) {
- + RxFreePool(entry->buf);
- + entry->buf = NULL;
- + }
- }
- out:
- if (entry) {
- @@ -411,6 +446,13 @@ NTSTATUS nfs41_SetSecurityInformation(
- InterlockedAdd64(&setacl.size, entry->u.Acl.buf_len);
- #endif
- + /* Invalidate cached ACL info */
- + if (nfs41_fobx->acl) {
- + RxFreePool(nfs41_fobx->acl);
- + nfs41_fobx->acl = NULL;
- + nfs41_fobx->acl_len = 0;
- + }
- +
- status = nfs41_UpcallWaitForReply(entry, pVNetRootContext->timeout);
- if (status) {
- /* Timeout - |nfs41_downcall()| will free |entry|+contents */
- --
- 2.45.1
- From 730dd47583bcb52a73c980b13a529d6291c374b5 Mon Sep 17 00:00:00 2001
- From: Roland Mainz <roland.mainz@nrubsig.org>
- Date: Thu, 26 Jun 2025 18:42:14 +0200
- Subject: [PATCH 2/2] daemon: symlink code should have a #define for debug
- level
- symlink code should have a #define for debug level instead of hardcoding
- the values.
- Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com>
- ---
- daemon/symlink.c | 25 ++++++++++++++-----------
- 1 file changed, 14 insertions(+), 11 deletions(-)
- diff --git a/daemon/symlink.c b/daemon/symlink.c
- index 29eea7f..78ab40c 100644
- --- a/daemon/symlink.c
- +++ b/daemon/symlink.c
- @@ -30,6 +30,9 @@
- #include "util.h"
- #include "daemon_debug.h"
- +/* |DPRINTF()| levels for acl logging */
- +#define SYMLLVL1 1
- +#define SYMLLVL2 2
- static int abs_path_link(
- OUT nfs41_abs_path *path,
- @@ -43,7 +46,7 @@ static int abs_path_link(
- const char *link_end = link + link_len;
- int status = NO_ERROR;
- - DPRINTF(2,
- + DPRINTF(SYMLLVL2,
- ("--> abs_path_link(path_pos='%s', link='%.*s', link_len=%d)\n",
- path_pos, (int)link_len, link, (int)link_len));
- @@ -103,12 +106,12 @@ out:
- path->len = (unsigned short)(path_pos - path->path);
- if (status) {
- - DPRINTF(2,
- + DPRINTF(SYMLLVL2,
- ("<-- abs_path_link(), status=%d\n",
- status));
- }
- else {
- - DPRINTF(2,
- + DPRINTF(SYMLLVL2,
- ("<-- abs_path_link(path='%.*s'), status=%d\n",
- (int)path->len, path->path, status));
- }
- @@ -136,7 +139,7 @@ int nfs41_symlink_target(
- goto out;
- }
- - DPRINTF(2, ("--> nfs41_symlink_target('%s', '%s')\n", path->path, link));
- + DPRINTF(SYMLLVL2, ("--> nfs41_symlink_target('%s', '%s')\n", path->path, link));
- /* append any components after the symlink */
- if (FAILED(StringCchCatA(link, NFS41_MAX_PATH_LEN,
- @@ -165,7 +168,7 @@ int nfs41_symlink_target(
- goto out;
- }
- out:
- - DPRINTF(2, ("<-- nfs41_symlink_target('%s') returning %d\n",
- + DPRINTF(SYMLLVL2, ("<-- nfs41_symlink_target('%s') returning %d\n",
- target->path, status));
- return status;
- }
- @@ -184,7 +187,7 @@ int nfs41_symlink_follow(
- file.path = &path;
- InitializeSRWLock(&path.lock);
- - DPRINTF(2, ("--> nfs41_symlink_follow('%s')\n", symlink->path->path));
- + DPRINTF(SYMLLVL2, ("--> nfs41_symlink_follow('%s')\n", symlink->path->path));
- do {
- if (++depth > NFS41_MAX_SYMLINK_DEPTH) {
- @@ -196,7 +199,7 @@ int nfs41_symlink_follow(
- status = nfs41_symlink_target(session, symlink, &path);
- if (status) goto out;
- - DPRINTF(2, ("looking up '%s'\n", path.path));
- + DPRINTF(SYMLLVL2, ("looking up '%s'\n", path.path));
- last_component(path.path, path.path + path.len, &file.name);
- @@ -208,7 +211,7 @@ int nfs41_symlink_follow(
- symlink = &file;
- } while (info->type == NF4LNK);
- out:
- - DPRINTF(2, ("<-- nfs41_symlink_follow() returning %d\n", status));
- + DPRINTF(SYMLLVL2, ("<-- nfs41_symlink_follow() returning %d\n", status));
- return status;
- }
- @@ -226,7 +229,7 @@ static int parse_symlink_get(unsigned char *buffer, uint32_t length,
- args->target_set = NULL;
- - DPRINTF(1,
- + DPRINTF(SYMLLVL1,
- ("parse_symlink_get: parsing NFS41_SYSOP_SYMLINK_GET: "
- "path='%s' target='%s'\n",
- args->path, args->target_set));
- @@ -254,7 +257,7 @@ static int handle_symlink_get(void *daemon_context, nfs41_upcall *upcall)
- goto out;
- }
- args->target_get.len = (unsigned short)len;
- - DPRINTF(2,
- + DPRINTF(SYMLLVL2,
- ("returning symlink target '%s'\n", args->target_get.path));
- out:
- @@ -316,7 +319,7 @@ static int parse_symlink_set(unsigned char *buffer, uint32_t length,
- status = get_name(&buffer, &length,
- (const char **)(&args->target_set));
- - DPRINTF(1,
- + DPRINTF(SYMLLVL1,
- ("parse_symlink_set: parsing NFS41_SYSOP_SYMLINK_SET: "
- "path='%s' target='%s'\n",
- args->path, args->target_set));
- --
- 2.45.1
msnfs41client: Fix kernel ACL cache+misc, 2025-06-26
Posted by Anonymous on Thu 26th Jun 2025 18:04
raw | new post
Submit a correction or amendment below (click here to make a fresh posting)
After submitting an amendment, you'll be able to view the differences between the old and new posts easily.