TFTP improvements

TFTP improvements

[Sascha Hauer]

This series contains some improvements for the TFTP support. The one
really important patch (and the reason I created this series) is 4/5.
Steffen made me aware that booting didn't work for him because his
boot script would always load a ramdisk image from tftp that is not
actually there. The reason was that we always claimed that a file is
present which was a concession to the lacking directory read support
on TFTP. We now actually test if a file is present before claiming
it is there.

Sascha

Sascha Hauer (5):
  fs: tftp: overhaul debugging
  fs: tftp: fix memory hole
  fs: tftp: fix return value
  fs: tftp: hide files which are actually not present on the server
  fs: tftp: improve file size handling

 fs/tftp.c | 97 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 31 deletions(-)

-- 
2.18.0


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

[PATCH 1/5] fs: tftp: overhaul debugging

[Sascha Hauer]

- use pr_* instead of debug()
- use pr_vdebug for the less interesting messages
- use pr_err for error messages
- print state as clear text and not as number

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/tftp.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index cc30c5eb8f..025edbfb86 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -16,6 +16,9 @@
  * GNU General Public License for more details.
  *
  */
+
+#define pr_fmt(fmt) "tftp: " fmt
+
 #include <common.h>
 #include <command.h>
 #include <net.h>
@@ -54,6 +57,7 @@
 #define TFTP_ERROR	5
 #define TFTP_OACK	6
 
+#define STATE_INVALID	0
 #define STATE_RRQ	1
 #define STATE_WRQ	2
 #define STATE_RDATA	3
@@ -94,6 +98,18 @@ static int tftp_truncate(struct device_d *dev, FILE *f, ulong size)
 	return 0;
 }
 
+static char *tftp_states[] = {
+	[STATE_INVALID] = "INVALID",
+	[STATE_RRQ] = "RRQ",
+	[STATE_WRQ] = "WRQ",
+	[STATE_RDATA] = "RDATA",
+	[STATE_WDATA] = "WDATA",
+	[STATE_OACK] = "OACK",
+	[STATE_WAITACK] = "WAITACK",
+	[STATE_LAST] = "LAST",
+	[STATE_DONE] = "DONE",
+};
+
 static int tftp_send(struct file_priv *priv)
 {
 	unsigned char *xp;
@@ -102,7 +118,7 @@ static int tftp_send(struct file_priv *priv)
 	unsigned char *pkt = net_udp_get_payload(priv->tftp_con);
 	int ret;
 
-	debug("%s: state %d\n", __func__, priv->state);
+	pr_vdebug("%s: state %s\n", __func__, tftp_states[priv->state]);
 
 	switch (priv->state) {
 	case STATE_RRQ:
@@ -206,7 +222,7 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 
 	pkt[len - 1] = 0;
 
-	debug("got OACK\n");
+	pr_debug("got OACK\n");
 #ifdef DEBUG
 	memory_display(pkt, 0, len, 1, 0);
 #endif
@@ -222,7 +238,7 @@ static void tftp_parse_oack(struct file_priv *priv, unsigned char *pkt, int len)
 			priv->filesize = simple_strtoul(val, NULL, 10);
 		if (!strcmp(opt, "blksize"))
 			priv->blocksize = simple_strtoul(val, NULL, 10);
-		debug("OACK opt: %s val: %s\n", opt, val);
+		pr_debug("OACK opt: %s val: %s\n", opt, val);
 		s = val + strlen(val) + 1;
 	}
 }
@@ -247,7 +263,7 @@ static void tftp_recv(struct file_priv *priv,
 	len -= 2;
 	pkt += 2;
 
-	debug("%s: opcode 0x%04x\n", __func__, opcode);
+	pr_vdebug("%s: opcode 0x%04x\n", __func__, opcode);
 
 	switch (opcode) {
 	case TFTP_RRQ:
@@ -260,7 +276,7 @@ static void tftp_recv(struct file_priv *priv,
 
 		priv->block = ntohs(*(uint16_t *)pkt);
 		if (priv->block != priv->last_block) {
-			debug("ack %d != %d\n", priv->block, priv->last_block);
+			pr_vdebug("ack %d != %d\n", priv->block, priv->last_block);
 			break;
 		}
 
@@ -303,7 +319,7 @@ static void tftp_recv(struct file_priv *priv,
 			priv->last_block = 0;
 
 			if (priv->block != 1) {	/* Assertion */
-				printf("error: First block is not block 1 (%d)\n",
+				pr_err("error: First block is not block 1 (%d)\n",
 					priv->block);
 				priv->err = -EINVAL;
 				priv->state = STATE_DONE;
@@ -330,8 +346,7 @@ static void tftp_recv(struct file_priv *priv,
 		break;
 
 	case TFTP_ERROR:
-		debug("\nTFTP error: '%s' (%d)\n",
-				pkt + 2, ntohs(*(uint16_t *)pkt));
+		pr_err("error: '%s' (%d)\n", pkt + 2, ntohs(*(uint16_t *)pkt));
 		switch (ntohs(*(uint16_t *)pkt)) {
 		case 1:
 			priv->err = -ENOENT;
@@ -512,7 +527,7 @@ static int tftp_write(struct device_d *_dev, FILE *f, const void *inbuf,
 	size_t size, now;
 	int ret;
 
-	debug("%s: %zu\n", __func__, insize);
+	pr_vdebug("%s: %zu\n", __func__, insize);
 
 	size = insize;
 
@@ -547,7 +562,7 @@ static int tftp_read(struct device_d *dev, FILE *f, void *buf, size_t insize)
 	size_t outsize = 0, now;
 	int ret;
 
-	debug("%s %zu\n", __func__, insize);
+	pr_vdebug("%s %zu\n", __func__, insize);
 
 	while (insize) {
 		now = kfifo_get(priv->fifo, buf, insize);
-- 
2.18.0


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

[PATCH 2/5] fs: tftp: fix memory hole

[Sascha Hauer]

dpath() returns a pointer to an allocated string, so we have to free it.
Put the pointer into our file private data and free it on close time.

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

diff --git a/fs/tftp.c b/fs/tftp.c
index 025edbfb86..bcb95bc1db 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -79,7 +79,7 @@ struct file_priv {
 	uint16_t last_block;
 	int state;
 	int err;
-	const char *filename;
+	char *filename;
 	int filesize;
 	uint64_t resend_timeout;
 	uint64_t progress_timeout;
@@ -139,7 +139,7 @@ static int tftp_send(struct file_priv *priv)
 				"%d%c"
 				"blksize%c"
 				"1432",
-				priv->filename, 0,
+				priv->filename + 1, 0,
 				0,
 				0,
 				TIMEOUT, 0,
@@ -374,16 +374,15 @@ static void tftp_handler(void *ctx, char *packet, unsigned len)
 }
 
 static struct file_priv *tftp_do_open(struct device_d *dev,
-		int accmode, const char *filename)
+		int accmode, struct dentry *dentry)
 {
+	struct fs_device_d *fsdev = dev_to_fs_device(dev);
 	struct file_priv *priv;
 	struct tftp_priv *tpriv = dev->priv;
 	int ret;
 
 	priv = xzalloc(sizeof(*priv));
 
-	filename++;
-
 	switch (accmode & O_ACCMODE) {
 	case O_RDONLY:
 		priv->push = 0;
@@ -408,7 +407,7 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 
 	priv->block = 1;
 	priv->err = -EINVAL;
-	priv->filename = filename;
+	priv->filename = dpath(dentry, fsdev->vfsmount.mnt_root);
 	priv->blocksize = TFTP_BLOCK_SIZE;
 	priv->block_requested = -1;
 
@@ -461,11 +460,8 @@ out:
 static int tftp_open(struct device_d *dev, FILE *file, const char *filename)
 {
 	struct file_priv *priv;
-	struct fs_device_d *fsdev = dev_to_fs_device(dev);
-
-	filename = dpath(file->dentry, fsdev->vfsmount.mnt_root);
 
-	priv = tftp_do_open(dev, file->flags, filename);
+	priv = tftp_do_open(dev, file->flags, file->dentry);
 	if (IS_ERR(priv))
 		return PTR_ERR(priv);
 
@@ -507,6 +503,7 @@ static int tftp_do_close(struct file_priv *priv)
 
 	net_unregister(priv->tftp_con);
 	kfifo_free(priv->fifo);
+	free(priv->filename);
 	free(priv->buf);
 	free(priv);
 
-- 
2.18.0


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

[PATCH 3/5] fs: tftp: fix return value

[Sascha Hauer]

When tftp_get_inode() fails it is a sign for a out of memory situtation
rather than an indicator for no space left on the filesystem, so return
-ENOMEM.

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

diff --git a/fs/tftp.c b/fs/tftp.c
index bcb95bc1db..0baf0c2890 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -653,7 +653,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry,
 
 	inode = tftp_get_inode(dir->i_sb, dir, S_IFREG | S_IRWXUGO);
 	if (!inode)
-		return ERR_PTR(-ENOSPC);
+		return ERR_PTR(-ENOMEM);
 
 	d_add(dentry, inode);
 
-- 
2.18.0


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

[PATCH 4/5] fs: tftp: hide files which are actually not present on the server

[Sascha Hauer]

In tftp_lookup we claimed that every desired file is there. This leads
to problems when a user only tests if a file is present and makes
decisions upon this information. Rather than claiming that all files
are present do a tftp_do_open() on the files and see if it is really
there.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/tftp.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/tftp.c b/fs/tftp.c
index 0baf0c2890..9274e931a2 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -391,14 +391,6 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
 	case O_WRONLY:
 		priv->push = 1;
 		priv->state = STATE_WRQ;
-		if (!(accmode & O_TRUNC)) {
-			/*
-			 * TFTP always truncates the existing file, so this
-			 * flag is mandatory when opening a file for writing.
-			 */
-			ret = -ENOSYS;
-			goto out;
-		}
 		break;
 	case O_RDWR:
 		ret = -ENOSYS;
@@ -646,10 +638,34 @@ static struct inode *tftp_get_inode(struct super_block *sb, const struct inode *
 	return inode;
 }
 
+static int tftp_create(struct inode *dir, struct dentry *dentry, umode_t mode)
+{
+	struct inode *inode;
+
+	inode = tftp_get_inode(dir->i_sb, dir, mode);
+	if (!inode)
+		return -EPERM;
+
+	inode->i_size = 0;
+
+	d_instantiate(dentry, inode);
+
+	return 0;
+}
+
 static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry,
 			    unsigned int flags)
 {
+	struct super_block *sb = dir->i_sb;
+	struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
 	struct inode *inode;
+	struct file_priv *priv;
+
+	priv = tftp_do_open(&fsdev->dev, O_RDONLY, dentry);
+	if (IS_ERR(priv))
+		return NULL;
+
+	tftp_do_close(priv);
 
 	inode = tftp_get_inode(dir->i_sb, dir, S_IFREG | S_IRWXUGO);
 	if (!inode)
@@ -663,6 +679,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry,
 static const struct inode_operations tftp_dir_inode_operations =
 {
 	.lookup = tftp_lookup,
+	.create = tftp_create,
 };
 
 static const struct super_operations tftp_ops;
-- 
2.18.0


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

[PATCH 5/5] fs: tftp: improve file size handling

[Sascha Hauer]

Previously we used FILE_SIZE_STREAM unconditionally. Instead, fill the
inode size with a valid filesize if we have one and only if not fall
back to FILE_SIZE_STREAM.

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

diff --git a/fs/tftp.c b/fs/tftp.c
index 9274e931a2..30256ada6e 100644
--- a/fs/tftp.c
+++ b/fs/tftp.c
@@ -458,7 +458,6 @@ static int tftp_open(struct device_d *dev, FILE *file, const char *filename)
 		return PTR_ERR(priv);
 
 	file->priv = priv;
-	file->size = SZ_2G;
 
 	return 0;
 }
@@ -619,7 +618,6 @@ static struct inode *tftp_get_inode(struct super_block *sb, const struct inode *
 
 	inode->i_ino = get_next_ino();
 	inode->i_mode = mode;
-	inode->i_size = FILE_SIZE_STREAM;
 
 	switch (mode & S_IFMT) {
 	default:
@@ -660,17 +658,25 @@ static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry,
 	struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
 	struct inode *inode;
 	struct file_priv *priv;
+	int filesize;
 
 	priv = tftp_do_open(&fsdev->dev, O_RDONLY, dentry);
 	if (IS_ERR(priv))
 		return NULL;
 
+	filesize = priv->filesize;
+
 	tftp_do_close(priv);
 
 	inode = tftp_get_inode(dir->i_sb, dir, S_IFREG | S_IRWXUGO);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
+	if (filesize)
+		inode->i_size = filesize;
+	else
+		inode->i_size = FILE_SIZE_STREAM;
+
 	d_add(dentry, inode);
 
 	return NULL;
-- 
2.18.0


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

Re: [PATCH 4/5] fs: tftp: hide files which are actually not present on the server

[Steffen Trumtrar]

On Thu, Aug 30, 2018 at 12:45:26PM +0200, Sascha Hauer wrote:
> In tftp_lookup we claimed that every desired file is there. This leads
> to problems when a user only tests if a file is present and makes
> decisions upon this information. Rather than claiming that all files
> are present do a tftp_do_open() on the files and see if it is really
> there.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  fs/tftp.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/tftp.c b/fs/tftp.c
> index 0baf0c2890..9274e931a2 100644
> --- a/fs/tftp.c
> +++ b/fs/tftp.c
> @@ -391,14 +391,6 @@ static struct file_priv *tftp_do_open(struct device_d *dev,
>  	case O_WRONLY:
>  		priv->push = 1;
>  		priv->state = STATE_WRQ;
> -		if (!(accmode & O_TRUNC)) {
> -			/*
> -			 * TFTP always truncates the existing file, so this
> -			 * flag is mandatory when opening a file for writing.
> -			 */
> -			ret = -ENOSYS;
> -			goto out;
> -		}
>  		break;
>  	case O_RDWR:
>  		ret = -ENOSYS;
> @@ -646,10 +638,34 @@ static struct inode *tftp_get_inode(struct super_block *sb, const struct inode *
>  	return inode;
>  }
>  
> +static int tftp_create(struct inode *dir, struct dentry *dentry, umode_t mode)
> +{
> +	struct inode *inode;
> +
> +	inode = tftp_get_inode(dir->i_sb, dir, mode);
> +	if (!inode)
> +		return -EPERM;
> +
> +	inode->i_size = 0;
> +
> +	d_instantiate(dentry, inode);
> +
> +	return 0;
> +}
> +
>  static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry,
>  			    unsigned int flags)
>  {
> +	struct super_block *sb = dir->i_sb;
> +	struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
>  	struct inode *inode;
> +	struct file_priv *priv;
> +
> +	priv = tftp_do_open(&fsdev->dev, O_RDONLY, dentry);
> +	if (IS_ERR(priv))
> +		return NULL;
> +
> +	tftp_do_close(priv);
>  
>  	inode = tftp_get_inode(dir->i_sb, dir, S_IFREG | S_IRWXUGO);
>  	if (!inode)
> @@ -663,6 +679,7 @@ static struct dentry *tftp_lookup(struct inode *dir, struct dentry *dentry,
>  static const struct inode_operations tftp_dir_inode_operations =
>  {
>  	.lookup = tftp_lookup,
> +	.create = tftp_create,
>  };
>  
>  static const struct super_operations tftp_ops;

Tested-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Thx,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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