Re: Hurd NFS translator
On 21/06/2025 18:18, Samuel Thibault wrote:
Better validate progressively.
Here are two further patches to add more NFSv3 functionality. They need
to be applied in sequence 3_3 then 3_4.
Patch v3_3 includes:
1) New code to process the reply from a file creation operation that
applies equally to CREATE3, MKDIR3, SYMLINK3 and MKNOD3. RFC1813 permits
a reply without the created file's handle and it might be necessary to
perform a LOOKUP3 on the created file post-creation. Debian Linux NFSV3
server actually always returns the handle when compiling gnumach however
I have tested the case where it might not by adding code (now removed)
to pretend the handle wasn't present in the reply. I expect therefore
that the 'skip_returned_stat' function that is added is rarely called.
2) Fixed netfs_attempt_mkdir() to process the NFSv3 reply properly.
Patch v3_4 includes:
1) Fixes netfs_attempt_read(): use the 64 bit value required for v3
'offset' and process opaque_data part of the reply properly.
2) Fixes netfs_attempt_create_file(): mutex_unlock for unlikely case of
RPC init failure and processing of v3 reply similarly to mkdir in v3_3 patch
3) The v3 netfs_attempt_create_file() uses the 'exclusive' creation
protocol which cannot specify the file permission bits or uid/gid. These
need to be applied post-creation using SETATTR3. There was a missing
call to do the chmod part which I've added. Previously the call to
chown that was present did not consider the return value. I have done so
but am now wondering if there was a specific reason to discard that error ?
Regards,
Mike.
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c 2025-07-13 07:01:38.000000000 +0100
+++ nfs.new/ops.c 2025-07-13 07:02:42.000000000 +0100
@@ -134,6 +134,70 @@
}
}
+/* advance the reply buffer beyond the 'post_op_attr' (v3) or
+ 'fattr' (v2). Populating 'struct stat' for the purposes of
+ advancing the correct number of bytes might seem wasteful
+ but it is not expected that this function will be called
+ often. */
+static int *
+skip_returned_stat (int *p)
+{
+ struct stat st;
+
+ if (protocol_version == 2)
+ return xdr_decode_fattr (p, &st);
+
+ int attrs_exist = ntohl (*p);
+ p++;
+
+ return (attrs_exist ? xdr_decode_fattr (p, &st) : p);
+}
+
+/* The reply to CREATE, MKDIR, SYMLINK and MKNOD in v3 all have
+ the same reply content. It is expected that 'np' is supplied
+ locked and any 'newnp' is also returned locked. */
+static error_t
+process_create_reply (struct iouser *cred,
+ struct node *np,
+ const char* name,
+ struct node **newnp,
+ int *p)
+{
+ assert_backtrace (protocol_version == 3);
+
+ error_t err = nfs_error_trans (ntohl (*p));
+ p++;
+
+ if (!err)
+ {
+ int handle_follows = ntohl (*p);
+ p++;
+
+ if (handle_follows)
+ {
+ p = xdr_decode_fhandle (p, newnp);
+ p = process_returned_stat (*newnp, p, 1);
+ }
+ else
+ /* These will be refetched in LOOKUP */
+ p = skip_returned_stat (p);
+
+ p = process_wcc_stat (np, p, 1);
+
+ if (!handle_follows)
+ {
+ err = netfs_attempt_lookup (cred, np, name, newnp);
+ /* netfs_attempt_lookup always unlocks 'np' so... */
+ pthread_mutex_lock (&np->lock);
+ }
+ }
+ else
+ {
+ p = process_wcc_stat (np, p, 1);
+ }
+
+ return err;
+}
/* Implement the netfs_validate_stat callback as described in
<hurd/netfs.h>. */
@@ -740,15 +804,23 @@
err = conduct_rpc (&rpcbuf, &p);
if (!err)
{
- err = nfs_error_trans (ntohl (*p));
- p++;
+ if (protocol_version == 2)
+ {
+ err = nfs_error_trans (ntohl (*p));
+ p++;
+
+ if (!err)
+ {
+ p = xdr_decode_fhandle (p, &newnp);
+ p = process_returned_stat (newnp, p, 1);
+ }
+ }
+ else
+ err = process_create_reply (cred, np, name, &newnp, p);
}
if (!err)
{
- p = xdr_decode_fhandle (p, &newnp);
- p = process_returned_stat (newnp, p, 1);
-
/* Did we set the owner correctly? If not, try, but ignore failures. */
if (!netfs_validate_stat (newnp, (struct iouser *) -1)
&& newnp->nn_stat.st_uid != owner)
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c 2025-07-13 07:02:42.000000000 +0100
+++ nfs.new/ops.c 2025-07-13 07:08:08.000000000 +0100
@@ -526,10 +526,17 @@
return errno;
p = xdr_encode_fhandle (p, &np->nn->handle);
- *(p++) = htonl (offset);
- *(p++) = htonl (thisamt);
if (protocol_version == 2)
- *(p++) = 0;
+ {
+ *(p++) = htonl (offset);
+ *(p++) = htonl (thisamt);
+ *(p++) = 0;
+ }
+ else
+ {
+ p = xdr_encode_64bit (p, offset);
+ *(p++) = htonl (thisamt);
+ }
err = conduct_rpc (&rpcbuf, &p);
if (!err)
@@ -553,8 +560,15 @@
if (protocol_version == 3)
{
+ size_t opaque_data_len;
+
eof = ntohl (*p);
p++;
+ opaque_data_len = ntohl (*p++);
+
+ /* opaque_len should surely equal trans_len, however... */
+ if (opaque_data_len < trans_len)
+ trans_len = opaque_data_len;
}
else
eof = (trans_len < thisamt);
@@ -1251,8 +1265,10 @@
p = nfs_initialize_rpc (NFSPROC_CREATE (protocol_version),
cred, 0, &rpcbuf, np, -1);
- if (! p)
+ if (! p) {
+ pthread_mutex_unlock (&np->lock);
return errno;
+ }
p = xdr_encode_fhandle (p, &np->nn->handle);
p = xdr_encode_string (p, name);
@@ -1270,37 +1286,36 @@
p = xdr_encode_create_state (p, mode, owner);
err = conduct_rpc (&rpcbuf, &p);
-
- pthread_mutex_unlock (&np->lock);
+ *newnp = 0;
if (!err)
{
- err = nfs_error_trans (ntohl (*p));
- p++;
- if (!err)
+ if (protocol_version == 2)
{
- p = xdr_decode_fhandle (p, newnp);
- p = process_returned_stat (*newnp, p, 1);
+ err = nfs_error_trans (ntohl (*p));
+ p++;
+ if (!err)
+ {
+ p = xdr_decode_fhandle (p, newnp);
+ p = process_returned_stat (*newnp, p, 1);
+ }
}
- if (err)
- *newnp = 0;
- if (protocol_version == 3)
+ else
+ err = process_create_reply (cred, np, name, newnp, p);
+
+ pthread_mutex_unlock (&np->lock);
+
+ if (*newnp && !netfs_validate_stat (*newnp, (struct iouser *) -1))
{
- if (*newnp)
- pthread_mutex_unlock (&(*newnp)->lock);
- pthread_mutex_lock (&np->lock);
- p = process_wcc_stat (np, p, 1);
- pthread_mutex_unlock (&np->lock);
- if (*newnp)
- pthread_mutex_lock (&(*newnp)->lock);
- }
+ if ((*newnp)->nn_stat.st_uid != owner)
+ err = netfs_attempt_chown ((struct iouser *) -1, *newnp, owner, (*newnp)->nn_stat.st_gid);
- if (*newnp && !netfs_validate_stat (*newnp, (struct iouser *) -1)
- && (*newnp)->nn_stat.st_uid != owner)
- netfs_attempt_chown ((struct iouser *) -1, *newnp, owner, (*newnp)->nn_stat.st_gid);
+ if (!err && (*newnp)->nn_stat.st_mode != mode)
+ err = netfs_attempt_chmod (cred, *newnp, mode);
+ }
}
else
- *newnp = 0;
+ pthread_mutex_unlock (&np->lock);
free (rpcbuf);
return err;
Reply to: