[PATCH 0/8] NFS fixes

[PATCH 0/8] NFS fixes

[Sascha Hauer]

This series fixes some issues with the NFS code. It was easy to confuse
the code by pressing ctrl-c during a file transfer. The next file
transfers then ended in corrupted files.

Sascha

Sascha Hauer (8):
  nfs: Add function to free packets
  nfs: Add missing free
  nfs: remove unnecessary check
  nfs: Fix rpc_check_reply() return value for stale packets
  nfs: Fix polling for packets
  nfs: queue received packets
  fs: nfs: don't maintain nfs dentries in dcache
  nfs: Do not allow to abort

 fs/nfs.c | 93 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 49 insertions(+), 44 deletions(-)

-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/8] nfs: Add function to free packets

[Sascha Hauer]

Add to function to free packets rather than freeing them directly. We
will introduce received packet qeueuing with one of the next patches,
this patch is meant to make it better readable.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index 0ad07aa3f2..0d098f2a66 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -397,6 +397,11 @@ static int rpc_check_reply(struct packet *pkt, int rpc_prog,
 	return 0;
 }
 
+static void nfs_free_packet(struct packet *packet)
+{
+	free(packet);
+}
+
 /*
  * rpc_req - synchronous RPC request
  */
@@ -472,7 +477,7 @@ again:
 				      npriv->rpc_id, &nfserr);
 		if (!ret) {
 			if (rpc_prog == PROG_NFS && nfserr) {
-				free(npriv->nfs_packet);
+				nfs_free_packet(npriv->nfs_packet);
 				return ERR_PTR(nfserr);
 			} else {
 				return npriv->nfs_packet;
@@ -505,7 +510,7 @@ static int rpc_lookup_req(struct nfs_priv *npriv, uint32_t prog, uint32_t ver)
 
 	port = ntoh32(net_read_uint32(nfs_packet->data + sizeof(struct rpc_reply)));
 
-	free(nfs_packet);
+	nfs_free_packet(nfs_packet);
 
 	return port;
 }
@@ -676,12 +681,12 @@ static int nfs_mount_req(struct nfs_priv *npriv)
 	if (npriv->rootfh.size > NFS3_FHSIZE) {
 		printf("%s: file handle too big: %lu\n",
 		       __func__, (unsigned long)npriv->rootfh.size);
-		free(nfs_packet);
+		nfs_free_packet(nfs_packet);
 		return -EIO;
 	}
 	memcpy(npriv->rootfh.data, p, npriv->rootfh.size);
 
-	free(nfs_packet);
+	nfs_free_packet(nfs_packet);
 
 	return 0;
 }
@@ -709,7 +714,7 @@ static void nfs_umount_req(struct nfs_priv *npriv)
 	nfs_packet = rpc_req(npriv, PROG_MOUNT, MOUNT_UMOUNT, data, len);
 
 	if (!IS_ERR(nfs_packet))
-		free(nfs_packet);
+		nfs_free_packet(nfs_packet);
 }
 
 /*
@@ -777,7 +782,7 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 
 	nfs_read_post_op_attr(p, inode);
 
-	free(nfs_packet);
+	nfs_free_packet(nfs_packet);
 
 	return 0;
 }
@@ -857,7 +862,7 @@ static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir)
 	len = (void *)nfs_packet->data + nfs_packet->len - (void *)p;
 	if (!len) {
 		printf("%s: huh, no payload left\n", __func__);
-		free(nfs_packet);
+		nfs_free_packet(nfs_packet);
 		return NULL;
 	}
 
@@ -865,7 +870,7 @@ static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir)
 
 	memcpy(buf, p, len);
 
-	free(nfs_packet);
+	nfs_free_packet(nfs_packet);
 
 	xdr_init(&dir->stream, buf, len);
 
@@ -942,13 +947,13 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset,
 	p += 2;
 
 	if (readlen && !rlen && !eof) {
-		free(nfs_packet);
+		nfs_free_packet(nfs_packet);
 		return -EIO;
 	}
 
 	kfifo_put(priv->fifo, (char *)p, rlen);
 
-	free(nfs_packet);
+	nfs_free_packet(nfs_packet);
 
 	return 0;
 }
@@ -1032,7 +1037,7 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 	*target = xzalloc(len + 1);
 	memcpy(*target, p, len);
 
-	free(nfs_packet);
+	nfs_free_packet(nfs_packet);
 
 	return 0;
 }
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/8] nfs: Add missing free

[Sascha Hauer]

Add forgotten packet free in error path.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs.c b/fs/nfs.c
index 0d098f2a66..eb316fe295 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -773,6 +773,7 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 
 	ninode->fh.size = ntoh32(net_read_uint32(p++));
 	if (ninode->fh.size > NFS3_FHSIZE) {
+		nfs_free_packet(nfs_packet);
 		debug("%s: file handle too big: %u\n", __func__,
 		      ninode->fh.size);
 		return -EIO;
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/8] nfs: remove unnecessary check

[Sascha Hauer]

In rpc_check_reply() pkt is never NULL, drop the unnecessary patch.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index eb316fe295..9956791820 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -366,9 +366,6 @@ static int rpc_check_reply(struct packet *pkt, int rpc_prog,
 
 	*nfserr = 0;
 
-	if (!pkt)
-		return -EAGAIN;
-
 	memcpy(&rpc, pkt->data, sizeof(rpc));
 
 	if (ntoh32(rpc.id) != rpc_id) {
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/8] nfs: Fix rpc_check_reply() return value for stale packets

[Sascha Hauer]

When we receive a packet with the previous rpc_id then we have
the comment "stale packet, wait a bit longer", but that's not what
the code does. rpc_check_reply() returns 0 in this case and the caller
then interprets the packet as valid.
Always return -EAGAIN for invalid rpc_ids. This lets the caller ignore
the packets as intented.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index 9956791820..1bfd36fcb6 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -368,13 +368,8 @@ static int rpc_check_reply(struct packet *pkt, int rpc_prog,
 
 	memcpy(&rpc, pkt->data, sizeof(rpc));
 
-	if (ntoh32(rpc.id) != rpc_id) {
-		if (rpc_id - ntoh32(rpc.id) == 1)
-			/* stale packet, wait a bit longer */
-			return 0;
-
-		return -EINVAL;
-	}
+	if (ntoh32(rpc.id) != rpc_id)
+		return -EAGAIN;
 
 	if (rpc.rstatus  ||
 	    rpc.verifier ||
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/8] nfs: Fix polling for packets

[Sascha Hauer]

It can happen that the first packet we receive is not the desired one.
In this case we have to poll for further packets. set nfs_state back to
STATE_START before polling for more packets, otherwise we check the same
packet again and again.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index 1bfd36fcb6..b070ca79d5 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -450,14 +450,17 @@ again:
 
 	nfs_timer_start = get_time_ns();
 
-	nfs_state = STATE_START;
+	while (1) {
+		nfs_state = STATE_START;
 
-	while (nfs_state != STATE_DONE) {
 		if (ctrlc())
 			return ERR_PTR(-EINTR);
 
 		net_poll();
 
+		if (nfs_state != STATE_DONE)
+			continue;
+
 		if (is_timeout(nfs_timer_start, NFS_TIMEOUT)) {
 			tries++;
 			if (tries == NFS_MAX_RESEND)
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/8] nfs: queue received packets

[Sascha Hauer]

It may happen that we receive more than one RPC packet before we come
along to check the result. Instead of handling just a single received
packet and possibly overwriting the last one queue the received RPC
packets.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 52 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index b070ca79d5..26ff354c37 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -131,6 +131,7 @@ struct nfs_fh {
 };
 
 struct packet {
+	struct list_head list;
 	int len;
 	char data[];
 };
@@ -145,7 +146,7 @@ struct nfs_priv {
 	unsigned manual_nfs_port:1;
 	uint32_t rpc_id;
 	struct nfs_fh rootfh;
-	struct packet *nfs_packet;
+	struct list_head packets;
 };
 
 struct file_priv {
@@ -175,10 +176,6 @@ static void nfs_set_fh(struct inode *inode, struct nfs_fh *fh)
 
 static uint64_t nfs_timer_start;
 
-static int nfs_state;
-#define STATE_DONE			1
-#define STATE_START			2
-
 /*
  * common types used in more than one request:
  *
@@ -384,13 +381,14 @@ static int rpc_check_reply(struct packet *pkt, int rpc_prog,
 	*nfserr = ntoh32(net_read_uint32(data));
 	*nfserr = -*nfserr;
 
-	debug("%s: state: %d, err %d\n", __func__, nfs_state, *nfserr);
+	debug("%s: err %d\n", __func__, *nfserr);
 
 	return 0;
 }
 
 static void nfs_free_packet(struct packet *packet)
 {
+	list_del(&packet->list);
 	free(packet);
 }
 
@@ -406,6 +404,7 @@ static struct packet *rpc_req(struct nfs_priv *npriv, int rpc_prog,
 	unsigned char *payload = net_udp_get_payload(npriv->con);
 	int nfserr;
 	int tries = 0;
+	struct packet *packet;
 
 	npriv->rpc_id++;
 
@@ -451,16 +450,11 @@ again:
 	nfs_timer_start = get_time_ns();
 
 	while (1) {
-		nfs_state = STATE_START;
-
 		if (ctrlc())
 			return ERR_PTR(-EINTR);
 
 		net_poll();
 
-		if (nfs_state != STATE_DONE)
-			continue;
-
 		if (is_timeout(nfs_timer_start, NFS_TIMEOUT)) {
 			tries++;
 			if (tries == NFS_MAX_RESEND)
@@ -468,19 +462,28 @@ again:
 			goto again;
 		}
 
-		ret = rpc_check_reply(npriv->nfs_packet, rpc_prog,
+		if (list_empty(&npriv->packets))
+			continue;
+
+		packet = list_first_entry(&npriv->packets, struct packet, list);
+
+		ret = rpc_check_reply(packet, rpc_prog,
 				      npriv->rpc_id, &nfserr);
-		if (!ret) {
+		if (ret == -EAGAIN) {
+			nfs_free_packet(packet);
+			continue;
+		} else if (ret) {
+			nfs_free_packet(packet);
+			return ERR_PTR(ret);
+		} else {
 			if (rpc_prog == PROG_NFS && nfserr) {
-				nfs_free_packet(npriv->nfs_packet);
+				nfs_free_packet(packet);
 				return ERR_PTR(nfserr);
 			} else {
-				return npriv->nfs_packet;
+				return packet;
 			}
 		}
 	}
-
-	return npriv->nfs_packet;
 }
 
 /*
@@ -954,16 +957,17 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset,
 	return 0;
 }
 
-static void nfs_handler(void *ctx, char *packet, unsigned len)
+static void nfs_handler(void *ctx, char *p, unsigned len)
 {
-	char *pkt = net_eth_to_udp_payload(packet);
+	char *pkt = net_eth_to_udp_payload(p);
 	struct nfs_priv *npriv = ctx;
+	struct packet *packet;
 
-	nfs_state = STATE_DONE;
+	packet = xmalloc(sizeof(*packet) + len);
+	memcpy(packet->data, pkt, len);
+	packet->len = len;
 
-	npriv->nfs_packet = xmalloc(sizeof(*npriv->nfs_packet) + len);
-	memcpy(npriv->nfs_packet->data, pkt, len);
-	npriv->nfs_packet->len = len;
+	list_add_tail(&packet->list, &npriv->packets);
 }
 
 static int nfs_truncate(struct device_d *dev, FILE *f, loff_t size)
@@ -1324,6 +1328,8 @@ static int nfs_probe(struct device_d *dev)
 
 	dev->priv = npriv;
 
+	INIT_LIST_HEAD(&npriv->packets);
+
 	debug("nfs: mount: %s\n", fsdev->backingstore);
 
 	path = strchr(tmp, ':');
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/8] fs: nfs: don't maintain nfs dentries in dcache

[Sascha Hauer]

nfs dentries may change underneath barebox, so do not maintain them in
the dcache.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs.c b/fs/nfs.c
index 26ff354c37..a5fc6467fb 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -1396,6 +1396,7 @@ static int nfs_probe(struct device_d *dev)
 	free(tmp);
 
 	sb->s_op = &nfs_ops;
+	sb->s_d_op = &no_revalidate_d_ops;
 
 	inode = new_inode(sb);
 	nfs_set_fh(inode, &npriv->rootfh);
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 8/8] nfs: Do not allow to abort

[Sascha Hauer]

When ctrl-c is pressed then ctrlc() will return true until
ctrlc_handled() is called. This means that once ctrl-c is pressed every
NFS operation will fail until the upper layer calls ctrlc_handled().
When for example we are doing a 'ls -l' on an NFS directory then after
a ctrl-c press not the 'ls -l' aborts, but instead the retrieving of the
directory entries which is not what we want.

Simply do not call ctrlc() in the fs layer. the NFS timeout is 2 seconds
which we have to wait until we have a chance to abort.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index a5fc6467fb..227a4866d7 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -450,9 +450,6 @@ again:
 	nfs_timer_start = get_time_ns();
 
 	while (1) {
-		if (ctrlc())
-			return ERR_PTR(-EINTR);
-
 		net_poll();
 
 		if (is_timeout(nfs_timer_start, NFS_TIMEOUT)) {
-- 
2.26.0.rc2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox