The F_GETLK fcntl can work with either read access or write access or
both. It can query F_RDLCK and F_WRLCK locks in either case.
However lockd currently treats F_GETLK similar to F_SETLK in that read
access is required to query an F_RDLCK lock and write access is required
to query a F_WRLCK lock.
This is wrong and can cause problems - e.g. when qemu accesses a
read-only (e.g. iso) filesystem image over NFS (though why it queries
if it can get a write lock - I don't know. But it does, and this works
with local filesystems).
So we need TEST requests to be handled differently. To do this:
- change nlm_do_fopen() to accept O_RDWR as a mode and in that case
succeed if either a O_RDONLY or O_WRONLY file can be opened.
- change nlm_lookup_file() to accept a mode argument from caller,
instead of deducing base on lock time, and pass that on to nlm_do_fopen()
- change nlm4svc_retrieve_args() and nlmsvc_retrieve_args() to detect
TEST requests and pass O_RDWR as a mode to nlm_lookup_file, passing
the same mode as before for other requests. Also set
lock->fl.c.flc_file to whichever file is available for TEST requests.
- change nlmsvc_testlock() to also not calculate the mode, but to use
whatever was stored in lock->fl.c.flc_file.
This behaviour of lockd - requesting O_WRONLY access to TEST for
exclusive locks - has been present at least since git history began.
However it was hidden until recently because knfsd ignored the access
requested by lockd and required only READ access for all locking
requests (unless the underlying filesystem provided an f_op->open
function which checked access permissions).
The commit mentioned in Fixes: below changed nfsd_permission() to NOT
override the access request for LOCK requests and this exposed the bug
that we are now fixing.
Note that there is another issue that this patch does not address.
The flock(.., LOCK_EX) call is permitted on a read-only file descriptor.
Linux NFS maps this to NLM locking as whole-file byte-range locks.
nfsd will see this as though it were fcntl( F_SETLK (F_WRLCK)) and will
now require write access, which it might not be able to get.
It is not clear if this is a problem in practice, or what the best
solution might be. So no attempt is made to address it.
Reported-by: Tj <tj.iam.tj@proton.me>
Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1128861
Fixes: 4cc9b9f2bf ("nfsd: refine and rename NFSD_MAY_LOCK")
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Now that all NLMv4 server-side procedures use XDR encoder and
decoder functions generated by xdrgen, the hand-written code in
fs/lockd/xdr4.c is no longer needed. This file contained the
original XDR processing logic that has been systematically
replaced throughout this series.
Remove the file and its Makefile reference to eliminate the
dead code. The helper function nlm4svc_set_file_lock_range()
is still needed by the generated code, so move it to xdr4.h
as an inline function where it remains accessible.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Public RPC server interfaces become cluttered when internal
XDR implementation details leak into them. The procedure count,
maximum XDR buffer size, and per-CPU call counters serve no
purpose outside the code that encodes and decodes NLM protocol
messages. Exposing these values through global headers creates
unnecessary coupling between the RPC dispatch logic and the
XDR layer.
Relocating the svc_version structure definitions confines this
implementation information to the files where XDR encoding and
decoding occur. In svc.c, the buffer size computation now reads
vs_xdrsize from the version structures rather than relying on a
preprocessor constant. This calculation occurs at service
initialization, after the linker has resolved the version
structure definitions. The dispatch function becomes non-static
because both the version structures and the dispatcher reside in
different translation units.
The NLMSVC_XDRSIZE macro is removed from xdr.h because buffer
size is now computed from the union of XDR argument and result
structures, matching the pattern used in other RPC services.
Version 1 and 3 share the same procedure table but maintain
separate counter arrays. Version 4 remains separate due to its
distinct procedure definitions.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Both client-side and server-side NLMv4 code convert lock byte ranges
from the wire format (start, length) to the kernel's file_lock format
(start, end). The current nlm4svc_set_file_lock_range() performs this
conversion, but the "svc" prefix incorrectly suggests server-only use,
and client code must include server-internal headers to access it.
Rename to lockd_set_file_lock_range4() and relocate to the shared
lockd.h header, making it accessible to both client and server code.
This eliminates the need for client code to include xdr4.h, reducing
coupling between the XDR implementation files.
While relocating the function, add input validation: clamp the
starting offset to OFFSET_MAX before use. Without this, a malformed
lock request with off > OFFSET_MAX results in fl_start > fl_end,
violating file_lock invariants and potentially causing incorrect
lock conflict detection.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
The NLM protocol constants and status codes in nlm.h are needed
only by lockd's internal implementation. NFS client code and
NFSD interact with lockd through the stable API in bind.h and
have no direct use for protocol-level definitions.
Exposing these definitions globally via bind.h creates unnecessary
coupling between lockd internals and its consumers. Moving nlm.h
from include/linux/lockd/ to fs/lockd/ clarifies the API boundary:
bind.h provides the lockd service interface, while nlm.h remains
available only to code within fs/lockd/ that implements the
protocol.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
The lockd subsystem unnecessarily exposes internal NLM XDR type
definitions through the global include path. These definitions
are not used by any code outside fs/lockd/, making them
inappropriate for include/linux/lockd/.
Moving xdr.h to fs/lockd/ narrows the API surface and clarifies
that these types are internal implementation details. The
comment in linux/lockd/bind.h stating xdr.h was needed for
"xdr-encoded error codes" is stale: no lockd API consumers use
those codes.
Forward declarations for struct nfs_fh and struct file_lock are
added to bind.h because their definitions were previously pulled
in transitively through xdr.h. Additionally, nfs3proc.c and
proc.c need explicit includes of filelock.h for FL_CLOSE and
for accessing struct file_lock members, respectively.
Built and tested with lockd client/server operations. No
functional change.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
The lockd include structure has unnecessary indirection. The header
include/linux/lockd/debug.h is consumed only by fs/lockd/lockd.h,
creating an extra compilation dependency and making the code harder
to navigate.
Fold the debug.h definitions directly into lockd.h and remove the
now-redundant header. This reduces the include tree depth and makes
the debug-related definitions easier to find when working on lockd
internals.
Build-tested with lockd built as module and built-in.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Headers placed in include/linux/ form part of the kernel's
internal API and signal to subsystem maintainers that other
parts of the kernel may depend on them. By moving lockd.h
into fs/lockd/, lockd becomes a more self-contained module
whose internal interfaces are clearly distinguished from its
public contract with the rest of the kernel. This relocation
addresses a long-standing XXX comment in the header itself
that acknowledged the file's misplacement. Future changes to
lockd internals can now proceed with confidence that external
consumers are not inadvertently coupled to implementation
details.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
