diff mbox

[for-next,V1,2/5] IB/core: Add ib_is_udata_cleared

Message ID 1449157463-25313-3-git-send-email-matanb@mellanox.com (mailing list archive)
State Superseded
Headers show

Commit Message

Matan Barak Dec. 3, 2015, 3:44 p.m. UTC 
Extending core and vendor verb commands require us to check that the
unknown part of the user's given command is all zeros.
Adding ib_is_udata_cleared in order to do so.

Signed-off-by: Matan Barak <matanb@mellanox.com>
Reviewed-by: Moshe Lazer <moshel@mellanox.com>
---
 include/rdma/ib_verbs.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

Comments

Haggai Eran Dec. 7, 2015, 1:18 p.m. UTC | #1 
On 12/03/2015 05:44 PM, Matan Barak wrote:
> Extending core and vendor verb commands require us to check that the
> unknown part of the user's given command is all zeros.
> Adding ib_is_udata_cleared in order to do so.
> 

Why not copy the data into kernel space and run memchr_inv() on it?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matan Barak Dec. 10, 2015, 2:59 p.m. UTC | #2 
On Mon, Dec 7, 2015 at 3:18 PM, Haggai Eran <haggaie@mellanox.com> wrote:
> On 12/03/2015 05:44 PM, Matan Barak wrote:
>> Extending core and vendor verb commands require us to check that the
>> unknown part of the user's given command is all zeros.
>> Adding ib_is_udata_cleared in order to do so.
>>
>
> Why not copy the data into kernel space and run memchr_inv() on it?
>

Probably less efficient, isn't it?
I know it isn't data path, but we'll execute this code in all extended
functions (sometimes even more than once).

> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Haggai Eran Dec. 10, 2015, 3:20 p.m. UTC | #3 
On 10/12/2015 16:59, Matan Barak wrote:
> On Mon, Dec 7, 2015 at 3:18 PM, Haggai Eran <haggaie@mellanox.com> wrote:
>> On 12/03/2015 05:44 PM, Matan Barak wrote:
>>> Extending core and vendor verb commands require us to check that the
>>> unknown part of the user's given command is all zeros.
>>> Adding ib_is_udata_cleared in order to do so.
>>>
>>
>> Why not copy the data into kernel space and run memchr_inv() on it?
>>
> 
> Probably less efficient, isn't it?
Why do you think it is less efficient?

I'm not sure calling copy_from_user multiple times is very efficient.
For once, you are calling access_ok multiple times. I guess it depends
on the amount of data you are copying.

> I know it isn't data path, but we'll execute this code in all extended
> functions (sometimes even more than once).
Do you think it is important enough to maintain our own copy of
memchr_inv()?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matan Barak Dec. 10, 2015, 5:29 p.m. UTC | #4 
On Thu, Dec 10, 2015 at 5:20 PM, Haggai Eran <haggaie@mellanox.com> wrote:
> On 10/12/2015 16:59, Matan Barak wrote:
>> On Mon, Dec 7, 2015 at 3:18 PM, Haggai Eran <haggaie@mellanox.com> wrote:
>>> On 12/03/2015 05:44 PM, Matan Barak wrote:
>>>> Extending core and vendor verb commands require us to check that the
>>>> unknown part of the user's given command is all zeros.
>>>> Adding ib_is_udata_cleared in order to do so.
>>>>
>>>
>>> Why not copy the data into kernel space and run memchr_inv() on it?
>>>
>>
>> Probably less efficient, isn't it?
> Why do you think it is less efficient?
>
> I'm not sure calling copy_from_user multiple times is very efficient.
> For once, you are calling access_ok multiple times. I guess it depends
> on the amount of data you are copying.
>

Isn't access_ok pretty cheap?
It calls __chk_range_not_ok which on x86 seems like a very cheap
function and __chk_user_ptr which is a compiler check.
I guess most kernel-user implementation will be pretty much in sync,
so we'll possibly call it for a few/dozens of bytes. In that case, I
think this implementation is a bit faster.

>> I know it isn't data path, but we'll execute this code in all extended
>> functions (sometimes even more than once).
> Do you think it is important enough to maintain our own copy of
> memchr_inv()?
>

True, I'm not sure it's important enough, but do you think it's that
complicated?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Haggai Eran Dec. 13, 2015, 3:47 p.m. UTC | #5 
On 10/12/2015 19:29, Matan Barak wrote:
> On Thu, Dec 10, 2015 at 5:20 PM, Haggai Eran <haggaie@mellanox.com> wrote:
>> On 10/12/2015 16:59, Matan Barak wrote:
>>> On Mon, Dec 7, 2015 at 3:18 PM, Haggai Eran <haggaie@mellanox.com> wrote:
>>>> On 12/03/2015 05:44 PM, Matan Barak wrote:
>>>>> Extending core and vendor verb commands require us to check that the
>>>>> unknown part of the user's given command is all zeros.
>>>>> Adding ib_is_udata_cleared in order to do so.
>>>>>
>>>>
>>>> Why not copy the data into kernel space and run memchr_inv() on it?
>>>>
>>>
>>> Probably less efficient, isn't it?
>> Why do you think it is less efficient?
>>
>> I'm not sure calling copy_from_user multiple times is very efficient.
>> For once, you are calling access_ok multiple times. I guess it depends
>> on the amount of data you are copying.
>>
> 
> Isn't access_ok pretty cheap?
> It calls __chk_range_not_ok which on x86 seems like a very cheap
> function and __chk_user_ptr which is a compiler check.
> I guess most kernel-user implementation will be pretty much in sync,
> so we'll possibly call it for a few/dozens of bytes. In that case, I
> think this implementation is a bit faster.
> 
>>> I know it isn't data path, but we'll execute this code in all extended
>>> functions (sometimes even more than once).
>> Do you think it is important enough to maintain our own copy of
>> memchr_inv()?
>>
> 
> True, I'm not sure it's important enough, but do you think it's that
> complicated?

It is complicated in my opinion. It is 67 lines of code, it's
architecture dependent and relies on preprocessor macros and conditional
code. I think this kind of stuff belongs in lib/string.c and not in the
RDMA stack.

Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matan Barak Dec. 14, 2015, 4:31 p.m. UTC | #6 
On Sun, Dec 13, 2015 at 5:47 PM, Haggai Eran <haggaie@mellanox.com> wrote:
> On 10/12/2015 19:29, Matan Barak wrote:
>> On Thu, Dec 10, 2015 at 5:20 PM, Haggai Eran <haggaie@mellanox.com> wrote:
>>> On 10/12/2015 16:59, Matan Barak wrote:
>>>> On Mon, Dec 7, 2015 at 3:18 PM, Haggai Eran <haggaie@mellanox.com> wrote:
>>>>> On 12/03/2015 05:44 PM, Matan Barak wrote:
>>>>>> Extending core and vendor verb commands require us to check that the
>>>>>> unknown part of the user's given command is all zeros.
>>>>>> Adding ib_is_udata_cleared in order to do so.
>>>>>>
>>>>>
>>>>> Why not copy the data into kernel space and run memchr_inv() on it?
>>>>>
>>>>
>>>> Probably less efficient, isn't it?
>>> Why do you think it is less efficient?
>>>
>>> I'm not sure calling copy_from_user multiple times is very efficient.
>>> For once, you are calling access_ok multiple times. I guess it depends
>>> on the amount of data you are copying.
>>>
>>
>> Isn't access_ok pretty cheap?
>> It calls __chk_range_not_ok which on x86 seems like a very cheap
>> function and __chk_user_ptr which is a compiler check.
>> I guess most kernel-user implementation will be pretty much in sync,
>> so we'll possibly call it for a few/dozens of bytes. In that case, I
>> think this implementation is a bit faster.
>>
>>>> I know it isn't data path, but we'll execute this code in all extended
>>>> functions (sometimes even more than once).
>>> Do you think it is important enough to maintain our own copy of
>>> memchr_inv()?
>>>
>>
>> True, I'm not sure it's important enough, but do you think it's that
>> complicated?
>
> It is complicated in my opinion. It is 67 lines of code, it's
> architecture dependent and relies on preprocessor macros and conditional
> code. I think this kind of stuff belongs in lib/string.c and not in the
> RDMA stack.
>

I'm not sure regarding the string.c location, as it deals with user
buffers, but in order not to
be dependent on this, I'll change this code to the following.

static inline bool ib_is_udata_cleared(struct ib_udata *udata,
                                       u8 cleared_char,
                                       size_t offset,
                                       size_t len)
{
        const void __user *p = udata->inbuf + offset;
        bool ret = false;
        u8 *buf;

        if (len > USHRT_MAX)
                return false;

        buf = kmalloc(len, GFP_KERNEL);
        if (!buf)
                return false;

        if (copy_from_user(buf, p, len))
                goto free;

        ret = !memchr_inv(buf, cleared_char, len);

free:
        kfree(buf);
        return ret;
}

> Haggai

Regards,
Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Haggai Eran Dec. 15, 2015, 2:13 p.m. UTC | #7 
On 14/12/2015 18:31, Matan Barak wrote:
> I'm not sure regarding the string.c location, as it deals with user
> buffers, but in order not to
> be dependent on this, I'll change this code to the following.
> 
> static inline bool ib_is_udata_cleared(struct ib_udata *udata,
>                                        u8 cleared_char,
>                                        size_t offset,
>                                        size_t len)
> {
>         const void __user *p = udata->inbuf + offset;
>         bool ret = false;
>         u8 *buf;
> 
>         if (len > USHRT_MAX)
>                 return false;
> 
>         buf = kmalloc(len, GFP_KERNEL);
>         if (!buf)
>                 return false;
> 
>         if (copy_from_user(buf, p, len))
>                 goto free;
> 
>         ret = !memchr_inv(buf, cleared_char, len);
> 
> free:
>         kfree(buf);
>         return ret;
> }

Looks good to me.

Thanks,
Haggai

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

Patch

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 31fb409..0ad89e3 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1947,6 +1947,73 @@  static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len
 	return copy_to_user(udata->outbuf, src, len) ? -EFAULT : 0;
 }
 
+#define IB_UDATA_ELEMENT_CLEARED(type, ptr, len, expected)		\
+		({type v;						\
+		  typeof(ptr) __ptr = ptr;				\
+									\
+		  ptr = (void *)ptr + sizeof(type);			\
+		  len -= sizeof(type);					\
+		  !copy_from_user(&v, __ptr, sizeof(v)) && (v == expected); })
+
+static inline bool ib_is_udata_cleared(struct ib_udata *udata,
+				       u8 cleared_char,
+				       size_t offset,
+				       size_t len)
+{
+	const void __user *p = udata->inbuf + offset;
+#ifdef CONFIG_64BIT
+	u64 expected = cleared_char;
+#else
+	u32 expected = cleared_char;
+#endif
+
+	if (len > USHRT_MAX)
+		return false;
+
+	if (len && (uintptr_t)p & 1)
+		if (!IB_UDATA_ELEMENT_CLEARED(u8, p, len, expected))
+			return false;
+
+	expected = expected << 8 | expected;
+	if (len >= 2 && (uintptr_t)p & 2)
+		if (!IB_UDATA_ELEMENT_CLEARED(u16, p, len, expected))
+			return false;
+
+	expected = expected << 16 | expected;
+#ifdef CONFIG_64BIT
+	if (len >= 4 && (uintptr_t)p & 4)
+		if (!IB_UDATA_ELEMENT_CLEARED(u32, p, len, expected))
+			return false;
+
+	expected = expected << 32 | expected;
+#define IB_UDATA_CLEAR_LOOP_TYPE	u64
+#else
+#define IB_UDATA_CLEAR_LOOP_TYPE	u32
+#endif
+	while (len >= sizeof(IB_UDATA_CLEAR_LOOP_TYPE))
+		if (!IB_UDATA_ELEMENT_CLEARED(IB_UDATA_CLEAR_LOOP_TYPE, p, len,
+					      expected))
+			return false;
+
+#ifdef CONFIG_64BIT
+	expected = expected >> 32;
+	if (len >= 4 && (uintptr_t)p & 4)
+		if (!IB_UDATA_ELEMENT_CLEARED(u32, p, len, expected))
+			return false;
+#endif
+	expected = expected >> 16;
+	if (len >= 2 && (uintptr_t)p & 2)
+		if (!IB_UDATA_ELEMENT_CLEARED(u16, p, len, expected))
+			return false;
+
+	expected = expected >> 8;
+	if (len)
+		if (!IB_UDATA_ELEMENT_CLEARED(u8, p, len, expected))
+			return false;
+
+	return true;
+}
+
 /**
  * ib_modify_qp_is_ok - Check that the supplied attribute mask
  * contains all required attributes and no attributes not allowed for