Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Bart Van Assche 
On Mon, Oct 24, 2011 at 7:33 AM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 +static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
 +       struct se_portal_group *se_tpg,
 +       const char *page,
 +       size_t count)
 +{
 +       struct srpt_port *sport = container_of(se_tpg, struct srpt_port, 
 port_tpg_1);
 +       unsigned long val;
 +       int ret;
 +
 +       ret = strict_strtoul(page, 0, val);

If the data page points at only consists of digits, the above
strict_strtoul() call will trigger a past-end-of-buffer read. Also,
isn't kstrto*() preferred over strict_strto*() ?

Bart.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Nicholas A. Bellinger 
On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
 On Mon, Oct 24, 2011 at 7:33 AM, Nicholas A. Bellinger
 n...@linux-iscsi.org wrote:
  +static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
  +   struct se_portal_group *se_tpg,
  +   const char *page,
  +   size_t count)
  +{
  +   struct srpt_port *sport = container_of(se_tpg, struct srpt_port, 
  port_tpg_1);
  +   unsigned long val;
  +   int ret;
  +
  +   ret = strict_strtoul(page, 0, val);
 
 If the data page points at only consists of digits, the above
 strict_strtoul() call will trigger a past-end-of-buffer read.

I don't understand what you mean here.  Can you provide a test case to
demonstrate please..?

 Also, isn't kstrto*() preferred over strict_strto*() ?
 

Not AFAIK.

--nab


--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Bart Van Assche 
On Mon, Oct 24, 2011 at 9:49 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
 On Mon, Oct 24, 2011 at 7:33 AM, Nicholas A. Bellinger
 n...@linux-iscsi.org wrote:
  +static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
  +       struct se_portal_group *se_tpg,
  +       const char *page,
  +       size_t count)
  +{
  +       struct srpt_port *sport = container_of(se_tpg, struct srpt_port, 
  port_tpg_1);
  +       unsigned long val;
  +       int ret;
  +
  +       ret = strict_strtoul(page, 0, val);

 If the data page points at only consists of digits, the above
 strict_strtoul() call will trigger a past-end-of-buffer read.

 I don't understand what you mean here.  Can you provide a test case to
 demonstrate please..?

echo -n 345 $configfs_path_of_parameter.

Bart.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Nicholas A. Bellinger 
On Mon, 2011-10-24 at 21:58 +0200, Bart Van Assche wrote:
 On Mon, Oct 24, 2011 at 9:49 PM, Nicholas A. Bellinger
 n...@linux-iscsi.org wrote:
  On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
  On Mon, Oct 24, 2011 at 7:33 AM, Nicholas A. Bellinger
  n...@linux-iscsi.org wrote:
   +static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
   +   struct se_portal_group *se_tpg,
   +   const char *page,
   +   size_t count)
   +{
   +   struct srpt_port *sport = container_of(se_tpg, struct srpt_port, 
   port_tpg_1);
   +   unsigned long val;
   +   int ret;
   +
   +   ret = strict_strtoul(page, 0, val);
 
  If the data page points at only consists of digits, the above
  strict_strtoul() call will trigger a past-end-of-buffer read.
 
  I don't understand what you mean here.  Can you provide a test case to
  demonstrate please..?
 
 echo -n 345 $configfs_path_of_parameter.
 

Still not sure what your getting at here..?

root@tifa:/sys/kernel/config/target/srpt/0x0002c903000e8acd/tpgt_1/attrib#
 ls -la
total 0
drwxr-xr-x 2 root root0 Oct 24 13:01 .
drwxr-xr-x 7 root root0 Oct 24 12:58 ..
-rw-r--r-- 1 root root 4096 Oct 24 12:58 srp_max_rdma_size
-rw-r--r-- 1 root root 4096 Oct 24 13:01 srp_max_rsp_size
-rw-r--r-- 1 root root 4096 Oct 24 12:58 srp_sq_size

root@tifa:/sys/kernel/config/target/srpt/0x0002c903000e8acd/tpgt_1/attrib#
 cat srp_max_rsp_size
256
root@tifa:/sys/kernel/config/target/srpt/0x0002c903000e8acd/tpgt_1/attrib#
 echo -n 240  srp_max_rsp_size
root@tifa:/sys/kernel/config/target/srpt/0x0002c903000e8acd/tpgt_1/attrib#
 cat srp_max_rsp_size
240


--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Bart Van Assche 
On Mon, Oct 24, 2011 at 10:05 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 On Mon, 2011-10-24 at 21:58 +0200, Bart Van Assche wrote:
 On Mon, Oct 24, 2011 at 9:49 PM, Nicholas A. Bellinger
 n...@linux-iscsi.org wrote:
  On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
  On Mon, Oct 24, 2011 at 7:33 AM, Nicholas A. Bellinger
  n...@linux-iscsi.org wrote:
   +static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
   +       struct se_portal_group *se_tpg,
   +       const char *page,
   +       size_t count)
   +{
   +       struct srpt_port *sport = container_of(se_tpg, struct 
   srpt_port, port_tpg_1);
   +       unsigned long val;
   +       int ret;
   +
   +       ret = strict_strtoul(page, 0, val);
 
  If the data page points at only consists of digits, the above
  strict_strtoul() call will trigger a past-end-of-buffer read.
 
  I don't understand what you mean here.  Can you provide a test case to
  demonstrate please..?

 echo -n 345 $configfs_path_of_parameter.

 Still not sure what your getting at here..?

Only the data in page[0..count-1] is guaranteed to be initialized.
strict_strtoul() will read until it either finds whitespace or a
binary zero, so if the data in page[] does neither contain whitespace
nor a binary zero then strict_strtoul() will read past the end of the
data in page[]. There may be any data at page[count], including a
valid digit.

Bart.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Bart Van Assche 
On Mon, Oct 24, 2011 at 9:49 PM, Nicholas A. Bellinger
n...@linux-iscsi.org wrote:
 On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
 Also, isn't kstrto*() preferred over strict_strto*() ?

 Not AFAIK.

This is what I found in the description of commit 33ee3b2:

kstrto*: converting strings to integers done (hopefully) right

1. simple_strto*() do not contain overflow checks and crufty,
   libc way to indicate failure.
2. strict_strto*() also do not have overflow checks but the name and
   comments pretend they do.
3. Both families have only long long and long variants,
   but users want strtou8()
4. Both simple and strict prefixes are wrong:
   Simple doesn't exactly say what's so simple, strict should not exist
   because conversion should be strict by default.

The solution is to use k prefix and add convertors for more types.
Enter
kstrtoull()
...

Bart.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Nicholas A. Bellinger 
On Mon, 2011-10-24 at 22:11 +0200, Bart Van Assche wrote:
 On Mon, Oct 24, 2011 at 10:05 PM, Nicholas A. Bellinger
 n...@linux-iscsi.org wrote:
  On Mon, 2011-10-24 at 21:58 +0200, Bart Van Assche wrote:
  On Mon, Oct 24, 2011 at 9:49 PM, Nicholas A. Bellinger
  n...@linux-iscsi.org wrote:
   On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
   On Mon, Oct 24, 2011 at 7:33 AM, Nicholas A. Bellinger
   n...@linux-iscsi.org wrote:
+static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
+   struct se_portal_group *se_tpg,
+   const char *page,
+   size_t count)
+{
+   struct srpt_port *sport = container_of(se_tpg, struct 
srpt_port, port_tpg_1);
+   unsigned long val;
+   int ret;
+
+   ret = strict_strtoul(page, 0, val);
  
   If the data page points at only consists of digits, the above
   strict_strtoul() call will trigger a past-end-of-buffer read.
  
   I don't understand what you mean here.  Can you provide a test case to
   demonstrate please..?
 
  echo -n 345 $configfs_path_of_parameter.
 
  Still not sure what your getting at here..?
 
 Only the data in page[0..count-1] is guaranteed to be initialized.
 strict_strtoul() will read until it either finds whitespace or a
 binary zero, so if the data in page[] does neither contain whitespace
 nor a binary zero then strict_strtoul() will read past the end of the
 data in page[]. There may be any data at page[count], including a
 valid digit.
 

That is part obvious.  The point your missing is that configfs is
already sanitizing the the incoming buffer in fs/configfs/file.c to work
with strict_strtoul() here:

static int
fill_write_buffer(struct configfs_buffer * buffer, const char __user * buf, 
size_t count)
{
int error;

if (!buffer-page)
buffer-page = (char *)__get_free_pages(GFP_KERNEL, 0);
if (!buffer-page)
return -ENOMEM;

if (count = SIMPLE_ATTR_SIZE)
count = SIMPLE_ATTR_SIZE - 1;
error = copy_from_user(buffer-page,buf,count);
buffer-needs_read_fill = 1;
/* if buf is assumed to contain a string, terminate it by \0,
 * so e.g. sscanf() can scan the string easily */
buffer-page[count] = 0;
return error ? -EFAULT : count;
}

--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-24 Thread Nicholas A. Bellinger 
On Mon, 2011-10-24 at 22:16 +0200, Bart Van Assche wrote:
 On Mon, Oct 24, 2011 at 9:49 PM, Nicholas A. Bellinger
 n...@linux-iscsi.org wrote:
  On Mon, 2011-10-24 at 21:44 +0200, Bart Van Assche wrote:
  Also, isn't kstrto*() preferred over strict_strto*() ?
 
  Not AFAIK.
 
 This is what I found in the description of commit 33ee3b2:
 
 kstrto*: converting strings to integers done (hopefully) right
 
 1. simple_strto*() do not contain overflow checks and crufty,
libc way to indicate failure.
 2. strict_strto*() also do not have overflow checks but the name and
comments pretend they do.
 3. Both families have only long long and long variants,
but users want strtou8()
 4. Both simple and strict prefixes are wrong:
Simple doesn't exactly say what's so simple, strict should not exist
because conversion should be strict by default.
 
 The solution is to use k prefix and add convertors for more types.
 Enter
 kstrtoull()
 ...

Fair enough.  I'll convert this separately after the merge considering
it's a very minor improvement, and not a bugfix.

--nab



--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/9] ib_srpt: Convert srp_max_rsp_size into per port configfs attribute

2011-10-23 Thread Nicholas A. Bellinger 
From: Nicholas Bellinger n...@linux-iscsi.org

This patch converts the srp_max_rsp_size module parameter into a per
port configfs attribute.  This includes adding the necessary bits for
show + store attributes w/ min/max bounds checking, and dropping
the legacy WARN_ON() as sport_port is not available for all callers
of srpt_alloc_ioctx_ring() and srpt_free_ioctx_ring().

It also drops the legacy srp_max_rsp_size module parameter checks
in srpt_init_module(), and adds new MAX_SRPT_RSP_SIZE = 1024 for
proper bounds checking in srpt_tpg_attrib_store_srp_max_rsp_size().

Reported-by: Roland Dreier rol...@purestorage.com
Cc: Roland Dreier rol...@purestorage.com
Cc: Bart Van Assche bvanass...@acm.org
Signed-off-by: Nicholas A. Bellinger n...@linux-iscsi.org
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |   67 +++-
 drivers/infiniband/ulp/srpt/ib_srpt.h |3 +
 2 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 80ef5af..f4a900b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -83,11 +83,6 @@ module_param(srp_max_req_size, int, 0444);
 MODULE_PARM_DESC(srp_max_req_size,
 Maximum size of SRP request messages in bytes.);
 
-static unsigned int srp_max_rsp_size = DEFAULT_MAX_RSP_SIZE;
-module_param(srp_max_rsp_size, int, 0444);
-MODULE_PARM_DESC(srp_max_rsp_size,
-Maximum size of SRP response messages in bytes.);
-
 static int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
 module_param(srpt_srq_size, int, 0444);
 MODULE_PARM_DESC(srpt_srq_size,
@@ -687,7 +682,6 @@ static struct srpt_ioctx **srpt_alloc_ioctx_ring(struct 
srpt_device *sdev,
 
WARN_ON(ioctx_size != sizeof(struct srpt_recv_ioctx)
 ioctx_size != sizeof(struct srpt_send_ioctx));
-   WARN_ON(dma_size != srp_max_req_size  dma_size != srp_max_rsp_size);
 
ring = kmalloc(ring_size * sizeof(ring[0]), GFP_KERNEL);
if (!ring)
@@ -717,8 +711,6 @@ static void srpt_free_ioctx_ring(struct srpt_ioctx 
**ioctx_ring,
 {
int i;
 
-   WARN_ON(dma_size != srp_max_req_size  dma_size != srp_max_rsp_size);
-
for (i = 0; i  ring_size; ++i)
srpt_free_ioctx(sdev, ioctx_ring[i], dma_size, dir);
kfree(ioctx_ring);
@@ -2384,7 +2376,8 @@ static void srpt_release_channel_work(struct work_struct 
*w)
 
srpt_free_ioctx_ring((struct srpt_ioctx **)ch-ioctx_ring,
 ch-sport-sdev, ch-rq_size,
-srp_max_rsp_size, DMA_TO_DEVICE);
+ch-sport-port_attrib.srp_max_rsp_size,
+DMA_TO_DEVICE);
 
spin_lock_irq(sdev-spinlock);
list_del(ch-list);
@@ -2568,7 +2561,8 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
ch-ioctx_ring = (struct srpt_send_ioctx **)
srpt_alloc_ioctx_ring(ch-sport-sdev, ch-rq_size,
  sizeof(*ch-ioctx_ring[0]),
- srp_max_rsp_size, DMA_TO_DEVICE);
+ ch-sport-port_attrib.srp_max_rsp_size,
+ DMA_TO_DEVICE);
if (!ch-ioctx_ring)
goto free_ch;
 
@@ -2676,8 +2670,8 @@ destroy_ib:
 free_ring:
srpt_free_ioctx_ring((struct srpt_ioctx **)ch-ioctx_ring,
 ch-sport-sdev, ch-rq_size,
-srp_max_rsp_size, DMA_TO_DEVICE);
-
+ch-sport-port_attrib.srp_max_rsp_size,
+DMA_TO_DEVICE);
 free_ch:
kfree(ch);
 
@@ -3294,6 +3288,7 @@ static void srpt_add_one(struct ib_device *device)
sport-sdev = sdev;
sport-port = i;
sport-port_attrib.srp_max_rdma_size = DEFAULT_MAX_RDMA_SIZE;
+   sport-port_attrib.srp_max_rsp_size = DEFAULT_MAX_RSP_SIZE;
INIT_WORK(sport-work, srpt_refresh_port_work);
INIT_LIST_HEAD(sport-port_acl_list);
spin_lock_init(sport-port_acl_lock);
@@ -3737,8 +3732,49 @@ static ssize_t srpt_tpg_attrib_store_srp_max_rdma_size(
 
 TF_TPG_ATTRIB_ATTR(srpt, srp_max_rdma_size, S_IRUGO | S_IWUSR);
 
+static ssize_t srpt_tpg_attrib_show_srp_max_rsp_size(
+   struct se_portal_group *se_tpg,
+   char *page)
+{
+   struct srpt_port *sport = container_of(se_tpg, struct srpt_port, 
port_tpg_1);
+
+   return sprintf(page, %u\n, sport-port_attrib.srp_max_rsp_size);
+}
+
+static ssize_t srpt_tpg_attrib_store_srp_max_rsp_size(
+   struct se_portal_group *se_tpg,
+   const char *page,
+   size_t count)
+{
+   struct srpt_port *sport = container_of(se_tpg, struct srpt_port, 
port_tpg_1);
+   unsigned long val;
+   int ret;
+
+   ret = strict_strtoul(page, 0, val);
+   if (ret  0) {
+   pr_err(strict_strtoul() failed