[PATCH 0/5] rbd: cleanups related to argument parsing

[PATCH 0/5] rbd: cleanups related to argument parsing

[Alex Elder]

This series affects the way arguments are parsed in rbd_add().
It first encapsulates the code into its own helper function.
Then it uses a few simple tokenization functions instead of
sscanf() to parse the string provided, which makes it possible
to do a better job of error checking the input.  It makes use
of the ability to check for certain invariants at build time
rather than runtime.  And by reworking what information gets
passed around, eliminates the need to allocate a buffer to
hold the monitor addresses.

					-Alex

[PATCH 1/5] rbd: encapsulate argument parsing for rbd_add()

[Alex Elder]

Move the code that parses the arguments provided to rbd_add() (which
are supplied via /sys/bus/rbd/add) into a separate function.

Also rename the "mon_dev_name" variable in rbd_add() to be
"mon_addrs".   The variable represents a list of one or more
comma-separated monitor IP addresses, each with an optional port
number.  I think "mon_addrs" captures that notion a little better.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   68 
+++++++++++++++++++++++++++++++-------------------
  1 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 29bbac1..3731a15 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2219,12 +2219,43 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
  	atomic_cmpxchg(&rbd_id_max, rbd_id, max_id);
  }

+/*
+ * This fills in the pool_name, obj, obj_len, snap_name, obj_len,
+ * rbd_dev, rbd_md_name, and name fields of the given rbd_dev, based
+ * on the list of monitor addresses and other options provided via
+ * /sys/bus/rbd/add.
+ */
+static int rbd_add_parse_args(struct rbd_device *rbd_dev,
+			      const char *buf,
+			      char *mon_addrs,
+			      char *options)
+{
+	if (sscanf(buf, "%" __stringify(RBD_MAX_OPT_LEN) "s "
+		   "%" __stringify(RBD_MAX_OPT_LEN) "s "
+		   "%" __stringify(RBD_MAX_POOL_NAME_LEN) "s "
+		   "%" __stringify(RBD_MAX_OBJ_NAME_LEN) "s"
+		   "%" __stringify(RBD_MAX_SNAP_NAME_LEN) "s",
+		   mon_addrs, options, rbd_dev->pool_name,
+		   rbd_dev->obj, rbd_dev->snap_name) < 4)
+		return -EINVAL;
+
+	if (rbd_dev->snap_name[0] == 0)
+		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+			sizeof RBD_SNAP_HEAD_NAME);
+
+	rbd_dev->obj_len = strlen(rbd_dev->obj);
+	snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
+		 rbd_dev->obj, RBD_SUFFIX);
+
+	return 0;
+}
+
  static ssize_t rbd_add(struct bus_type *bus,
  		       const char *buf,
  		       size_t count)
  {
  	struct rbd_device *rbd_dev;
-	char *mon_dev_name = NULL;
+	char *mon_addrs = NULL;
  	char *options = NULL;
  	struct ceph_osd_client *osdc;
  	int rc = -ENOMEM;
@@ -2235,8 +2266,8 @@ static ssize_t rbd_add(struct bus_type *bus,
  	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
  	if (!rbd_dev)
  		goto err_nomem;
-	mon_dev_name = kmalloc(count, GFP_KERNEL);
-	if (!mon_dev_name)
+	mon_addrs = kmalloc(count, GFP_KERNEL);
+	if (!mon_addrs)
  		goto err_nomem;
  	options = kmalloc(count, GFP_KERNEL);
  	if (!options)
@@ -2250,30 +2281,15 @@ static ssize_t rbd_add(struct bus_type *bus,
  	/* generate unique id: find highest unique id, add one */
  	rbd_id_get(rbd_dev);

+	/* Fill in the device name, now that we have its id. */
+	snprintf(rbd_dev->name, DEV_NAME_LEN, RBD_DRV_NAME "%d", rbd_dev->id);
+
  	/* parse add command */
-	if (sscanf(buf, "%" __stringify(RBD_MAX_OPT_LEN) "s "
-		   "%" __stringify(RBD_MAX_OPT_LEN) "s "
-		   "%" __stringify(RBD_MAX_POOL_NAME_LEN) "s "
-		   "%" __stringify(RBD_MAX_OBJ_NAME_LEN) "s"
-		   "%" __stringify(RBD_MAX_SNAP_NAME_LEN) "s",
-		   mon_dev_name, options, rbd_dev->pool_name,
-		   rbd_dev->obj, rbd_dev->snap_name) < 4) {
-		rc = -EINVAL;
+	rc = rbd_add_parse_args(rbd_dev, buf, mon_addrs, options);
+	if (rc)
  		goto err_put_id;
-	}
-
-	if (rbd_dev->snap_name[0] == 0)
-		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
-			sizeof RBD_SNAP_HEAD_NAME);
-
-	rbd_dev->obj_len = strlen(rbd_dev->obj);
-	snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
-		 rbd_dev->obj, RBD_SUFFIX);
-
-	/* initialize rest of new object */
-	snprintf(rbd_dev->name, DEV_NAME_LEN, RBD_DRV_NAME "%d", rbd_dev->id);

-	rbd_dev->rbd_client = rbd_get_client(mon_dev_name, options);
+	rbd_dev->rbd_client = rbd_get_client(mon_addrs, options);
  	if (IS_ERR(rbd_dev->rbd_client)) {
  		rc = PTR_ERR(rbd_dev->rbd_client);
  		goto err_put_id;
@@ -2314,7 +2330,7 @@ err_out_bus:

  	rbd_bus_del_dev(rbd_dev);
  	kfree(options);
-	kfree(mon_dev_name);
+	kfree(mon_addrs);
  	return rc;

  err_out_blkdev:
@@ -2325,7 +2341,7 @@ err_put_id:
  	rbd_id_put(rbd_dev);
  err_nomem:
  	kfree(options);
-	kfree(mon_dev_name);
+	kfree(mon_addrs);
  	kfree(rbd_dev);

  	dout("Error adding device %s\n", buf);
-- 
1.7.5.4

[PATCH 2/5] rbd: don't use sscanf() in rbd_add_parse_args()

[Alex Elder]

Make use of a few simple helper routines to parse the arguments
rather than sscanf().  This will treat both missing and too-long
arguments as invalid input (rather than silently truncating the
input in the too-long case).  In time this can also be used by
rbd_add() to use the passed-in buffer in place, rather than copying
its contents into new buffers.

It appears to me that the sscanf() previously used would not
correctly handle a supplied snapshot--the two final "%s" conversion
specifications were not separated by a space, and I'm not sure
how sscanf() handles that situation.  It may not be well-defined.
So that may be a bug this change fixes (but I didn't verify that).

The sizes of the mon_addrs and options buffers are now passed to
rbd_add_parse_args(), so they can be supplied to copy_token().

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   99 
+++++++++++++++++++++++++++++++++++++++++++-------
  1 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 3731a15..d2157a7 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2220,6 +2220,53 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
  }

  /*
+ * Skips over white space at *buf, and updates *buf to point to the
+ * first found non-space character (if any). Returns the length of
+ * the token (string of non-white space characters) found.
+ */
+static inline size_t next_token(const char **buf)
+{
+        /*
+        * These are the characters that produce nonzero for
+        * isspace() in the "C" and "POSIX" locales.
+        */
+        const char *spaces = " \f\n\r\t\v";
+
+        *buf += strspn(*buf, spaces);	/* Find start of token */
+
+	return strcspn(*buf, spaces);   /* Return token length */
+}
+
+/*
+ * Finds the next token in *buf, and if the provided token buffer is
+ * big enough, copies the found token into it.  The result, if
+ * copied, is guaranteed to be terminated with '\0'.
+ *
+ * Returns the length of the token found (not including the '\0').
+ * Return value will be 0 if no token is found, and it will be >=
+ * token_size if the token would not fit.
+ *
+ * The *buf pointer will be updated point beyond the end of the
+ * found token.  Note that this occurs even if the token buffer is
+ * too small to hold it.
+ */
+static inline size_t copy_token(const char **buf,
+				char *token,
+				size_t token_size)
+{
+        size_t len;
+
+	len = next_token(buf);
+	if (len < token_size) {
+		memcpy(token, *buf, len);
+		*(token + len) = '\0';
+	}
+	*buf += len;
+
+        return len;
+}
+
+/*
   * This fills in the pool_name, obj, obj_len, snap_name, obj_len,
   * rbd_dev, rbd_md_name, and name fields of the given rbd_dev, based
   * on the list of monitor addresses and other options provided via
@@ -2228,25 +2275,48 @@ static void rbd_id_put(struct rbd_device *rbd_dev)
  static int rbd_add_parse_args(struct rbd_device *rbd_dev,
  			      const char *buf,
  			      char *mon_addrs,
-			      char *options)
-{
-	if (sscanf(buf, "%" __stringify(RBD_MAX_OPT_LEN) "s "
-		   "%" __stringify(RBD_MAX_OPT_LEN) "s "
-		   "%" __stringify(RBD_MAX_POOL_NAME_LEN) "s "
-		   "%" __stringify(RBD_MAX_OBJ_NAME_LEN) "s"
-		   "%" __stringify(RBD_MAX_SNAP_NAME_LEN) "s",
-		   mon_addrs, options, rbd_dev->pool_name,
-		   rbd_dev->obj, rbd_dev->snap_name) < 4)
+			      size_t mon_addrs_size,
+			      char *options,
+			      size_t options_size)
+{
+	size_t	len;
+
+	/* The first four tokens are required */
+
+	len = copy_token(&buf, mon_addrs, mon_addrs_size);
+	if (!len || len >= mon_addrs_size)
  		return -EINVAL;

-	if (rbd_dev->snap_name[0] == 0)
-		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
-			sizeof RBD_SNAP_HEAD_NAME);
+	len = copy_token(&buf, options, options_size);
+	if (!len || len >= options_size)
+		return -EINVAL;
+
+	len = copy_token(&buf, rbd_dev->pool_name, sizeof rbd_dev->pool_name);
+	if (!len || len >= sizeof rbd_dev->pool_name)
+		return -EINVAL;
+
+	len = copy_token(&buf, rbd_dev->obj, sizeof rbd_dev->obj);
+	if (!len || len >= sizeof rbd_dev->obj)
+		return -EINVAL;
+
+	/* We have the object length in hand, save it. */
+
+	rbd_dev->obj_len = len;

-	rbd_dev->obj_len = strlen(rbd_dev->obj);
  	snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
  		 rbd_dev->obj, RBD_SUFFIX);

+	/*
+	 * The snapshot name is optional, but it's an error if it's
+	 * too long.  If no snapshot is supplied, fill in the default.
+	 */
+	len = copy_token(&buf, rbd_dev->snap_name, sizeof rbd_dev->snap_name);
+	if (!len)
+		memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
+			sizeof RBD_SNAP_HEAD_NAME);
+	else if (len >= sizeof rbd_dev->snap_name)
+		return -EINVAL;
+
  	return 0;
  }

@@ -2285,7 +2355,8 @@ static ssize_t rbd_add(struct bus_type *bus,
  	snprintf(rbd_dev->name, DEV_NAME_LEN, RBD_DRV_NAME "%d", rbd_dev->id);

  	/* parse add command */
-	rc = rbd_add_parse_args(rbd_dev, buf, mon_addrs, options);
+	rc = rbd_add_parse_args(rbd_dev, buf, mon_addrs, count,
+				options, count);
  	if (rc)
  		goto err_put_id;

-- 
1.7.5.4

[PATCH 3/5] rbd: do a few checks at build time

[Alex Elder]

This is a bit gratuitous, but there are a few things that can be
verified at build time rather than run time, so do that.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   15 ++++++++++++---
  1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d2157a7..0602ed9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -53,7 +53,14 @@

  #define RBD_SNAP_HEAD_NAME	"-"

+/*
+ * An RBD device name will be "rbd#", where the "rbd" comes from
+ * RBD_DRV_NAME above, and # is a unique integer identifier.
+ * MAX_INT_FORMAT_WIDTH is used in ensuring DEV_NAME_LEN is big
+ * enough to hold all possible device names.
+ */
  #define DEV_NAME_LEN		32
+#define MAX_INT_FORMAT_WIDTH	((5 * sizeof (int)) / 2 + 1)

  #define RBD_NOTIFY_TIMEOUT_DEFAULT 10

@@ -2303,8 +2310,9 @@ static int rbd_add_parse_args(struct rbd_device 
*rbd_dev,

  	rbd_dev->obj_len = len;

-	snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
-		 rbd_dev->obj, RBD_SUFFIX);
+	BUILD_BUG_ON(RBD_MAX_MD_NAME_LEN
+				< RBD_MAX_OBJ_NAME_LEN + sizeof RBD_SUFFIX);
+	sprintf(rbd_dev->obj_md_name, "%s%s", rbd_dev->obj, RBD_SUFFIX);

  	/*
  	 * The snapshot name is optional, but it's an error if it's
@@ -2352,7 +2360,8 @@ static ssize_t rbd_add(struct bus_type *bus,
  	rbd_id_get(rbd_dev);

  	/* Fill in the device name, now that we have its id. */
-	snprintf(rbd_dev->name, DEV_NAME_LEN, RBD_DRV_NAME "%d", rbd_dev->id);
+	BUILD_BUG_ON(DEV_NAME_LEN < sizeof RBD_DRV_NAME + MAX_INT_FORMAT_WIDTH);
+	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);

  	/* parse add command */
  	rc = rbd_add_parse_args(rbd_dev, buf, mon_addrs, count,
-- 
1.7.5.4

[PATCH 4/5] rbd: have rbd_parse_args() report found mon_addrs size

[Alex Elder]

The argument parsing routine already computes the size of the
mon_addrs buffer it extracts from the "command."  Pass it to the
caller so it can use it to provide the length to rbd_get_client().

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   20 +++++++++++++-------
  1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 0602ed9..1cc7614 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -391,7 +391,9 @@ static int parse_rbd_opts_token(char *c, void *private)
   * Get a ceph client with specific addr and configuration, if one does
   * not exist create it.
   */
-static struct rbd_client *rbd_get_client(const char *mon_addr, char 
*options)
+static struct rbd_client *rbd_get_client(const char *mon_addr,
+					 size_t mon_addr_len,
+					 char *options)
  {
  	struct rbd_client *rbdc;
  	struct ceph_options *opt;
@@ -404,7 +406,7 @@ static struct rbd_client *rbd_get_client(const char 
*mon_addr, char *options)
  	rbd_opts->notify_timeout = RBD_NOTIFY_TIMEOUT_DEFAULT;

  	opt = ceph_parse_options(options, mon_addr,
-				mon_addr + strlen(mon_addr),
+				mon_addr + mon_addr_len,
  				parse_rbd_opts_token, rbd_opts);
  	if (IS_ERR(opt)) {
  		kfree(rbd_opts);
@@ -2282,7 +2284,7 @@ static inline size_t copy_token(const char **buf,
  static int rbd_add_parse_args(struct rbd_device *rbd_dev,
  			      const char *buf,
  			      char *mon_addrs,
-			      size_t mon_addrs_size,
+			      size_t *mon_addrs_size,
  			      char *options,
  			      size_t options_size)
  {
@@ -2290,9 +2292,10 @@ static int rbd_add_parse_args(struct rbd_device 
*rbd_dev,

  	/* The first four tokens are required */

-	len = copy_token(&buf, mon_addrs, mon_addrs_size);
-	if (!len || len >= mon_addrs_size)
+	len = copy_token(&buf, mon_addrs, *mon_addrs_size);
+	if (!len || len >= *mon_addrs_size)
  		return -EINVAL;
+	*mon_addrs_size = len + 1;

  	len = copy_token(&buf, options, options_size);
  	if (!len || len >= options_size)
@@ -2334,6 +2337,7 @@ static ssize_t rbd_add(struct bus_type *bus,
  {
  	struct rbd_device *rbd_dev;
  	char *mon_addrs = NULL;
+	size_t mon_addrs_size;
  	char *options = NULL;
  	struct ceph_osd_client *osdc;
  	int rc = -ENOMEM;
@@ -2364,12 +2368,14 @@ static ssize_t rbd_add(struct bus_type *bus,
  	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);

  	/* parse add command */
-	rc = rbd_add_parse_args(rbd_dev, buf, mon_addrs, count,
+	mon_addrs_size = count;
+	rc = rbd_add_parse_args(rbd_dev, buf, mon_addrs, &mon_addrs_size,
  				options, count);
  	if (rc)
  		goto err_put_id;

-	rbd_dev->rbd_client = rbd_get_client(mon_addrs, options);
+	rbd_dev->rbd_client = rbd_get_client(mon_addrs, mon_addrs_size - 1,
+						options);
  	if (IS_ERR(rbd_dev->rbd_client)) {
  		rc = PTR_ERR(rbd_dev->rbd_client);
  		goto err_put_id;
-- 
1.7.5.4

[PATCH 5/5] rbd: don't allocate mon_addrs buffer in rbd_add()

[Alex Elder]

The mon_addrs buffer in rbd_add is used to hold a copy of the
monitor IP addresses supplied via /sys/bus/rbd/add.  That is
passed to rbd_get_client(), which never modifies it (nor do
any of the functions it gets passed to thereafter)--the mon_addr
parameter to rbd_get_client() is a pointer to constant data, so it
can't be modifed.  Furthermore, rbd_get_client() has the length of
the mon_addrs buffer and that is used to ensure nothing goes beyond
its end.

Based on all this, there is no reason that a buffer needs to
be used to hold a copy of the mon_addrs provided via
/sys/bus/rbd/add.   Instead, the location within that passed-in
buffer can be provided, along with the length of the "token"
therein which represents the monitor IP's.

A small change to rbd_add_parse_args() allows the address within the
buffer to be passed back, and the length is already returned.  This
now means that, at least from the perspective of this interface,
there is no such thing as a list of monitor addresses that is too
long.

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  drivers/block/rbd.c |   21 +++++++++------------
  1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 1cc7614..bc539cd 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2283,7 +2283,7 @@ static inline size_t copy_token(const char **buf,
   */
  static int rbd_add_parse_args(struct rbd_device *rbd_dev,
  			      const char *buf,
-			      char *mon_addrs,
+			      const char **mon_addrs,
  			      size_t *mon_addrs_size,
  			      char *options,
  			      size_t options_size)
@@ -2292,10 +2292,13 @@ static int rbd_add_parse_args(struct rbd_device 
*rbd_dev,

  	/* The first four tokens are required */

-	len = copy_token(&buf, mon_addrs, *mon_addrs_size);
-	if (!len || len >= *mon_addrs_size)
+	len = next_token(&buf);
+	if (!len)
  		return -EINVAL;
  	*mon_addrs_size = len + 1;
+	*mon_addrs = buf;
+
+	buf += len;

  	len = copy_token(&buf, options, options_size);
  	if (!len || len >= options_size)
@@ -2336,8 +2339,8 @@ static ssize_t rbd_add(struct bus_type *bus,
  		       size_t count)
  {
  	struct rbd_device *rbd_dev;
-	char *mon_addrs = NULL;
-	size_t mon_addrs_size;
+	const char *mon_addrs = NULL;
+	size_t mon_addrs_size = 0;
  	char *options = NULL;
  	struct ceph_osd_client *osdc;
  	int rc = -ENOMEM;
@@ -2348,9 +2351,6 @@ static ssize_t rbd_add(struct bus_type *bus,
  	rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL);
  	if (!rbd_dev)
  		goto err_nomem;
-	mon_addrs = kmalloc(count, GFP_KERNEL);
-	if (!mon_addrs)
-		goto err_nomem;
  	options = kmalloc(count, GFP_KERNEL);
  	if (!options)
  		goto err_nomem;
@@ -2368,8 +2368,7 @@ static ssize_t rbd_add(struct bus_type *bus,
  	sprintf(rbd_dev->name, "%s%d", RBD_DRV_NAME, rbd_dev->id);

  	/* parse add command */
-	mon_addrs_size = count;
-	rc = rbd_add_parse_args(rbd_dev, buf, mon_addrs, &mon_addrs_size,
+	rc = rbd_add_parse_args(rbd_dev, buf, &mon_addrs, &mon_addrs_size,
  				options, count);
  	if (rc)
  		goto err_put_id;
@@ -2416,7 +2415,6 @@ err_out_bus:

  	rbd_bus_del_dev(rbd_dev);
  	kfree(options);
-	kfree(mon_addrs);
  	return rc;

  err_out_blkdev:
@@ -2427,7 +2425,6 @@ err_put_id:
  	rbd_id_put(rbd_dev);
  err_nomem:
  	kfree(options);
-	kfree(mon_addrs);
  	kfree(rbd_dev);

  	dout("Error adding device %s\n", buf);
-- 
1.7.5.4

Re: [PATCH 0/5] rbd: cleanups related to argument parsing

[Sage Weil]

These all look good.

Reviewed-by: Sage Weil <sage@newdream.net>

On Tue, 28 Feb 2012, Alex Elder wrote:

> This series affects the way arguments are parsed in rbd_add().
> It first encapsulates the code into its own helper function.
> Then it uses a few simple tokenization functions instead of
> sscanf() to parse the string provided, which makes it possible
> to do a better job of error checking the input.  It makes use
> of the ability to check for certain invariants at build time
> rather than runtime.  And by reworking what information gets
> passed around, eliminates the need to allocate a buffer to
> hold the monitor addresses.
> 
> 					-Alex
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>