IB/core: extended query device caps cleanup for v3.19
Subsystems (2)
cover IB/core: extended query device caps cleanup for v3.19
Hi,
Following discussions in thread "[PATCH v3 06/17] IB/core: Add support for extended query device caps" [1] and further comments on previous patch in "[PATCH 3/4] IB/uverbs: ex_query_device: check request's comp_mask" thread [2], I'm proposing an updated patchset to implement a slighly different behavior to handle the request parameters in ib_uverbs_ex_query_device() in order to restore the current behavior of ib_copy_to_udata().
I think we found an agreement on the scheme to be implemented in Haggai Eran's response [3]:
- input's comp_mask is currently not used, so it must be 0;
- the response buffer size is checked so that the base answer can be returned without being truncated;
- the extended feature (odp) information are returned if, and only if, there's enough space in the response buffer, allowing older programs to get only the features they would expect. Newer programs are expected to provide more space, but as the response's comp_mask will describe only the available fields, newer program cannot be surprized to not get information when run on older kernel (if we want to support this use case).
I feel even more confident this behavior would allow a better maintainability. Additionally, it's still looking more like the behavior already implemented by other InfiniBand/RDMA kernel <-> userspace interfaces.
I hope this would go in v3.19 before its release so that the extended QUERY_DEVICE uverbs won't hurt us being part of an official release:
in it's current shape it won't be easy to support it.
Regards.
Changes from v0 [4]
- don't use input's comp_mask to conditionnaly build the response
- ensure input's comp_mask is set 0
[1] http://mid.gmane.org/[email protected]
[2] http://mid.gmane.org/063956366559d6919693aabb324bab83d676bc28.1421931555.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[3] http://mid.gmane.org/54C902E4.5010600-VPRAkNaXOzVWk0Htik3J/[email protected]
[4] http://mid.gmane.org/cover.1421931555.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Yann Droneaud (5):
IB/uverbs: ex_query_device: answer must not depend on request's
comp_mask
IB/uverbs: ex_query_device: check request's comp_mask IB/uverbs: ex_query_device: answer must depend on response buffer's
size
IB/uverbs: ex_query_device: no need to clear the whole structure IB/core: ib_copy_to_udata(): don't silently truncate response
drivers/infiniband/core/uverbs_cmd.c | 39 +++++++++++++++++++++++++----------- include/rdma/ib_verbs.h | 5 +---- 2 files changed, 28 insertions(+), 16 deletions(-)
Signature · 1 line
2.1.0
PATCH 1/5 IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveAs specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's comp_mask should describe the request data: it's describe the availability of extended fields in the request. Conversely, the response's comp_mask should describe the presence of extended fields in the response.
So this patch changes ib_uverbs_ex_query_device() function to always return the maximum supported features, currently only IB_USER_VERBS_EX_QUERY_DEVICE_ODP per commit 860f10a799c8 ("IB/core:
Add flags for on demand paging support"), regardless of the value of request's comp_mask.
--- drivers/infiniband/core/uverbs_cmd.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
drivers/infiniband/core/uverbs_cmd.c +8 -10
@@ -3324,17 +3324,15 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, resp.comp_mask =0 ;#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING -if (cmd.comp_mask &IB_USER_VERBS_EX_QUERY_DEVICE_ODP ) { - resp.odp_caps.general_caps = attr.odp_caps.general_caps; - resp.odp_caps.per_transport_caps.rc_odp_caps = - attr.odp_caps.per_transport_caps.rc_odp_caps; - resp.odp_caps.per_transport_caps.uc_odp_caps = - attr.odp_caps.per_transport_caps.uc_odp_caps; - resp.odp_caps.per_transport_caps.ud_odp_caps = - attr.odp_caps.per_transport_caps.ud_odp_caps; - resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; - } + resp.odp_caps.general_caps = attr.odp_caps.general_caps; + resp.odp_caps.per_transport_caps.rc_odp_caps = + attr.odp_caps.per_transport_caps.rc_odp_caps; + resp.odp_caps.per_transport_caps.uc_odp_caps = + attr.odp_caps.per_transport_caps.uc_odp_caps; + resp.odp_caps.per_transport_caps.ud_odp_caps = + attr.odp_caps.per_transport_caps.ud_odp_caps;#endif + resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; err =ib_copy_to_udata (ucore, &resp,sizeof (resp));if (err)
Signature · 1 line
2.1.0
PATCH 2/5 IB/uverbs: ex_query_device: check request's comp_mask
In reply to a message not in the archiveThis patch ensures the extended QUERY_DEVICE uverbs request's comp_mask has only known and supported bits (currently none).
If userspace set unknown features bits, -EINVAL will be returned, ensuring current programs are not allowed to set random feature
bits: such bits could enable new extended features in future kernel versions and those features can trigger a behavior not unsupported by the older programs or make the newer kernels return an error for a request which was valid on older kernels.
Additionally, returning an error for unsupported feature would allow userspace to probe/discover which extended features are currently supported by a kernel.
--- drivers/infiniband/core/uverbs_cmd.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +3 -0
@@ -3312,6 +3312,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,if (err)return err; +if (cmd.comp_mask) +return -EINVAL ; +if (cmd.reserved)return -EINVAL ;
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/5 IB/uverbs: ex_query_device: answer must depend on response buffer's size
In reply to a message not in the archiveAs specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's comp_mask should describe the request data: it's describe the availability of extended fields in the request. Conversely, the response's comp_mask should describe the presence of extended fields in the response.
So instead of silently truncating extended QUERY_DEVICE uverb's response, see commit 5a77abf9a97a ("IB/core: Add support for extended query device caps")), this patch makes function ib_uverbs_ex_query_device() check the available space in the response buffer against the minimal response structure and fail with -ENOSPC if this base structure cannot be returned to
userspace: it's required to be able to return the comp_mask's value, at least.
For extended features, currently only IB_USER_VERBS_EX_QUERY_DEVICE_ODP per commit 860f10a799c8 ("IB/core: Add flags for on demand paging support"), the extension's data is returned if the needed space is available in the response buffer: it is expected that newer userspace program pass a bigger response buffer so that it can retrieve extended features. The comp_mask value will match the fields that were effectively returned to userspace.
In the end:
- userspace won't get truncated responses;
- newer kernel would be able to support older binaries and older binaries will work flawlessly with newer kernel;
- additionally, newer binaries would even be able to support older kernel, as far as they don't set new feature bit in request's comp_mask.
Note: as offsetof() is used to retrieve the size of the lower chunk of the response, beware that it only works if the upper chunk is right after, without any implicit padding. And, as the size of the latter chunk is added to the base size, implicit padding at the end of the structure is not taken in account. Both point must be taken in account when extending the uverbs functionalities.
--- drivers/infiniband/core/uverbs_cmd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/infiniband/core/uverbs_cmd.c +12 -1
@@ -3302,6 +3302,7 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,struct ib_uverbs_ex_query_device cmd;struct ib_device_attr attr;struct ib_device *device; +size_t resp_len;int err; device = file->device->ib_dev;
@@ -3318,6 +3319,11 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,if (cmd.reserved)return -EINVAL ; + resp_len = offsetof(typeof (resp ), odp_caps); + +if (ucore->outlen < resp_len) +return -ENOSPC ; + err = device->query_device (device, &attr);if (err)return err;
@@ -3326,6 +3332,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,copy_query_dev_fields (file, &resp.base, &attr); resp.comp_mask =0 ; +if (ucore->outlen < resp_len +sizeof (resp.odp_caps)) + goto end; +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING resp.odp_caps.general_caps = attr.odp_caps.general_caps; resp.odp_caps.per_transport_caps.rc_odp_caps =
@@ -3336,8 +3345,10 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, attr.odp_caps.per_transport_caps.ud_odp_caps;#endif resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; + resp_len +=sizeof (resp.odp_caps); - err =ib_copy_to_udata (ucore, &resp,sizeof (resp)); +end: + err =ib_copy_to_udata (ucore, &resp, resp_len);if (err)return err;
Signature · 1 line
2.1.0
PATCH 4/5 IB/uverbs: ex_query_device: no need to clear the whole structure
In reply to a message not in the archiveAs only the requested fields are set and copied to userspace, there's no need to clear the content of the response structure beforehand.
--- drivers/infiniband/core/uverbs_cmd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/infiniband/core/uverbs_cmd.c +4 -1
@@ -3328,9 +3328,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,if (err)return err; -memset (&resp,0 ,sizeof (resp));copy_query_dev_fields (file, &resp.base, &attr); resp.comp_mask =0 ; + resp.reserved =0 ;if (ucore->outlen < resp_len +sizeof (resp.odp_caps)) goto end;
@@ -3343,6 +3343,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, attr.odp_caps.per_transport_caps.uc_odp_caps; resp.odp_caps.per_transport_caps.ud_odp_caps = attr.odp_caps.per_transport_caps.ud_odp_caps; + resp.odp_caps.reserved =0 ; +#else +memset (&resp.odp_caps,0 ,sizeof (resp.odp_caps));#endif resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; resp_len +=sizeof (resp.odp_caps);
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 5/5 IB/core: ib_copy_to_udata(): don't silently truncate response
In reply to a message not in the archiveWhile ib_copy_to_udata() should check for the available output space as already proposed in some other patches [1][2][3], the changes brought by commit 5a77abf9a97a ("IB/core: Add support for extended query device caps") are silently truncating the data to be written to userspace if the output buffer is not large enough to hold the response data.
Silently truncating the response is not a reliable behavior as userspace is not given any hint about this truncation: userspace is leaved with garbage to play with.
Not checking the response buffer size and writing past the userspace buffer is no good either, but it's the current behavior.
So this patch revert the particular change on ib_copy_to_udata() as a better behavior is implemented in the upper level function ib_uverbs_ex_query_device().
[1] "[PATCH 00/22] infiniband: improve userspace input check"
http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[2] "[PATCH 03/22] infiniband: ib_copy_from_udata(): check input length"
[3] "[PATCH 04/22] infiniband: ib_copy_to_udata(): check output length"
--- include/rdma/ib_verbs.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
include/rdma/ib_verbs.h +1 -4
@@ -1707,10 +1707,7 @@ static inline int ib_copy_from_udata(void *dest, struct ib_udata *udata, size_tstatic inline int ib_copy_to_udata (struct ib_udata *udata,void *src,size_t len) { -size_t copy_sz; - - copy_sz =min_t (size_t, len, udata->outlen); -return copy_to_user (udata->outbuf, src, copy_sz) ? -EFAULT :0 ; +return copy_to_user (udata->outbuf, src, len) ? -EFAULT :0 ; }/**
Signature · 6 lines
2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe
Reviewed-byRe: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 6 lines · Yann Droneaud
As specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's
comp_maskshould describe the request data: it's describe the availability of extended fields in the request. Conversely, the response'scomp_maskshould describe the presence of extended fields in the response.
Roland: I agree with Yann, these patches need to go in, or the ODP patches reverted.
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3327
Quoted 2 lines
#ifdef
CONFIG_INFINIBAND_ON_DEMAND_PAGINGdrivers/infiniband/core/uverbs_cmd.c +0
-1-if (cmd.comp_mask &IB_USER_VERBS_EX_QUERY_DEVICE_ODP ) {
Absolutely, a query output should never depend on the input comp_mask.
Quoted 8 lines
drivers/infiniband/core/uverbs_cmd.c +0
-8- resp.odp_caps.general_caps = attr.odp_caps.general_caps; - resp.odp_caps.per_transport_caps.rc_odp_caps = - attr.odp_caps.per_transport_caps.rc_odp_caps; - resp.odp_caps.per_transport_caps.uc_odp_caps = - attr.odp_caps.per_transport_caps.uc_odp_caps; - resp.odp_caps.per_transport_caps.ud_odp_caps = - attr.odp_caps.per_transport_caps.ud_odp_caps; - resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ;
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3335
Quoted 10 lines
drivers/infiniband/core/uverbs_cmd.c +8
-1- } + resp.odp_caps.general_caps = attr.odp_caps.general_caps; + resp.odp_caps.per_transport_caps.rc_odp_caps = + attr.odp_caps.per_transport_caps.rc_odp_caps; + resp.odp_caps.per_transport_caps.uc_odp_caps = + attr.odp_caps.per_transport_caps.uc_odp_caps; + resp.odp_caps.per_transport_caps.ud_odp_caps = + attr.odp_caps.per_transport_caps.ud_odp_caps;#endif + resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ;
Not sure about this - if 0 is a valid null answer for all the _caps then it is fine, and the comp_mask bit should just be removed as the size alone should be enough.
This looks wrong however:
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
Quoted 4 lines
drivers/infiniband/core/uverbs_cmd.c +0
-0err =ib_copy_to_udata (ucore, &resp,sizeof (resp));if (err)return err;return 0 ;
Why isn't this returning the filled structure size to userspace? This would seem to be very urgently wrong to me? Am I missing something?
Patch 3 probably should gain:
- return 0;
+ return resp_len;
This patch looks like an improvement to me:
Jason
Jason Gunthorpe
Reviewed-byRe: [PATCH v1 2/5] IB/uverbs: ex_query_device: check request's comp_mask
In reply to a message not in the archiveQuoted 2 lines · Yann Droneaud
This patch ensures the extended
QUERY_DEVICEuverbs request'scomp_maskhas only known and supported bits (currently none).
I think I would be happy to see the input comp_mask removed entirely. I can't see a possible use for input data to a QUERY command that wouldn't be better served by creating a new command
But forcing the value to 0 seems reasonable as well.
Also, the _ex varients were supposed to be supersets of the base call, so it is wrong that query_device_ex doesn't return all the same data as query_device, layed out so that the original response structure is a prefix of the extended response structure.
The other _ex calls in verbs were designed that way so it will be surprising to the user that this one is different.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/5] IB/uverbs: ex_query_device: answer must depend on response buffer's size
In reply to a message not in the archiveQuoted 6 lines · Yann Droneaud
As specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's
comp_maskshould describe the request data: it's describe the availability of extended fields in the request. Conversely, the response'scomp_maskshould describe the presence of extended fields in the response.
This makes sense to me as well.
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3353
Quoted 6 lines
drivers/infiniband/core/uverbs_cmd.c +2
-1- err =ib_copy_to_udata (ucore, &resp,sizeof (resp)); +end: + err =ib_copy_to_udata (ucore, &resp, resp_len);if (err)return err;
I think resp_len should be returned, not 0?
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe
Reviewed-byRe: [PATCH v1 4/5] IB/uverbs: ex_query_device: no need to clear the whole structure
In reply to a message not in the archiveQuoted 3 lines · Yann Droneaud
As only the requested fields are set and copied to userspace, there's no need to clear the content of the response structure beforehand.
Agreed.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveHi,
Quoted 8 lines · Jason Gunthorpe
Quoted 6 lines · Yann Droneaud
As specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's
comp_maskshould describe the request data: it's describe the availability of extended fields in the request. Conversely, the response'scomp_maskshould describe the presence of extended fields in the response.
Quoted 8 lines
Roland: I agree with Yann, these patches need to go in, or the ODP patches reverted.
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3327
Quoted 2 lines
#ifdef
CONFIG_INFINIBAND_ON_DEMAND_PAGINGdrivers/infiniband/core/uverbs_cmd.c +0
-1-if (cmd.comp_mask &IB_USER_VERBS_EX_QUERY_DEVICE_ODP ) {Absolutely, a query output should never depend on the input
comp_mask.
We should keep this in mind then.
Quoted 14 lines
Quoted 14 lines
drivers/infiniband/core/uverbs_cmd.c +5
-9- resp.odp_caps.general_caps = attr.odp_caps.general_caps; - resp.odp_caps.per_transport_caps.rc_odp_caps = - attr.odp_caps.per_transport_caps.rc_odp_caps; - resp.odp_caps.per_transport_caps.uc_odp_caps = - attr.odp_caps.per_transport_caps.uc_odp_caps; - resp.odp_caps.per_transport_caps.ud_odp_caps = - attr.odp_caps.per_transport_caps.ud_odp_caps; - resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; - } + resp.odp_caps.general_caps = attr.odp_caps.general_caps; + resp.odp_caps.per_transport_caps.rc_odp_caps = + attr.odp_caps.per_transport_caps.rc_odp_caps; + resp.odp_caps.per_transport_caps.uc_odp_caps = + attr.odp_caps.per_transport_caps.uc_odp_caps;
Quoted 9 lines
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3335
Quoted 4 lines
drivers/infiniband/core/uverbs_cmd.c +3
-0+ resp.odp_caps.per_transport_caps.ud_odp_caps = + attr.odp_caps.per_transport_caps.ud_odp_caps;#endif + resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ;Not sure about this - if 0 is a valid null answer for all the
_capsthen it is fine, and thecomp_maskbit should just be removed as the size alone should be enough.
That's difficult to say. But I hope 0 are valids answers in this case.
Anyway, the response's comp_mask is needed, as it's the sole way for the userspace to know the size of the response. See below.
Quoted 7 lines
This looks wrong however:
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
Quoted 4 lines
drivers/infiniband/core/uverbs_cmd.c +0
-0err =ib_copy_to_udata (ucore, &resp,sizeof (resp));if (err)return err;return 0 ;
Quoted 8 lines
Why isn't this returning the filled structure size to userspace? This would seem to be very urgently wrong to me? Am I missing something?
Patch 3 probably should gain:
- return 0;
+ returnresp_len;
The write() syscall must return the size buffer passed to it, or less, but in such case it would ask for trouble as userspace would be allowed to write() the remaining bytes.
Returning a size bigger than the one passed to write() is not acceptable and would break any expectation.
Quoted 4 lines
This patch looks like an improvement to me:
Thanks a lot.
Regards.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 5 lines · Yann Droneaud
The
write()syscall must return the size buffer passed to it, or less, but in such case it would ask for trouble as userspace would be allowed towrite()the remaining bytes. Returning a size bigger than the one passed towrite()is not acceptable and would break any expectation.
By that logic the 0 return is still wrong, and it should be ucore->in_len
But I think we can return less without risking anything breaking, it already violates the invariant associated with write() - it mutates the buffer passed in!
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 2/5] IB/uverbs: ex_query_device: check request's comp_mask
In reply to a message not in the archiveHi,
Quoted 10 lines · Jason Gunthorpe
Quoted 2 lines · Yann Droneaud
This patch ensures the extended
QUERY_DEVICEuverbs request'scomp_maskhas only known and supported bits (currently none).I think I would be happy to see the input
comp_maskremoved entirely. I can't see a possible use for input data to a QUERY command that wouldn't be better served by creating a new commandBut forcing the value to 0 seems reasonable as well.
I cannot forsee the future, but having at least one unused bit available allow for any kind of yet unknown extension: having this bit/these bits checked now, permit fixing mistake later.
So let it be.
Quoted 2 lines
Thanks.
Quoted 8 lines
Also, the
_exvarients were supposed to be supersets of the base call, so it is wrong thatquery_device_exdoesn't return all the same data asquery_device, layed out so that the original response structure is a prefix of the extended response structure.The other
_excalls in verbs were designed that way so it will be surprising to the user that this one is different.
It seems to me it has the layout you're expecting:
224 struct ib_uverbs_ex_query_device_resp {
225 struct ib_uverbs_query_device_resp base;
226 __u32 comp_mask;
227 __u32 reserved;
228 struct ib_uverbs_odp_caps odp_caps;
229 };Regards.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/5] IB/uverbs: ex_query_device: answer must depend on response buffer's size
In reply to a message not in the archiveHi,
Quoted 10 lines · Jason Gunthorpe
Quoted 6 lines · Yann Droneaud
As specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's
comp_maskshould describe the request data: it's describe the availability of extended fields in the request. Conversely, the response'scomp_maskshould describe the presence of extended fields in the response.This makes sense to me as well.
Quoted 9 lines
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3353
Quoted 6 lines
drivers/infiniband/core/uverbs_cmd.c +2
-1- err =ib_copy_to_udata (ucore, &resp,sizeof (resp)); +end: + err =ib_copy_to_udata (ucore, &resp, resp_len);if (err)return err;I think
resp_lenshould be returned, not 0?
As said previously it's not possible to use the effective size of the response as the return value of the write() syscall: the syscall must return the size of the input buffer, not the size of the output buffer, otherwise it would break the semantics of the write() syscall.
Regards.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 2/5] IB/uverbs: ex_query_device: check request's comp_mask
In reply to a message not in the archiveQuoted 4 lines · Jason Gunthorpe
Also, the
_exvarients were supposed to be supersets of the base call, so it is wrong thatquery_device_exdoesn't return all the same data asquery_device, layed out so that the original response structure is a prefix of the extended response structure.
Never mind this, I see the copy_query_dev_fields call now!
Thanks,
Jason
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 27 lines · Yann Droneaud
Quoted 16 lines · Jason Gunthorpe
Quoted 14 lines · Yann Droneaud
drivers/infiniband/core/uverbs_cmd.c +5
-9- resp.odp_caps.general_caps = attr.odp_caps.general_caps; - resp.odp_caps.per_transport_caps.rc_odp_caps = - attr.odp_caps.per_transport_caps.rc_odp_caps; - resp.odp_caps.per_transport_caps.uc_odp_caps = - attr.odp_caps.per_transport_caps.uc_odp_caps; - resp.odp_caps.per_transport_caps.ud_odp_caps = - attr.odp_caps.per_transport_caps.ud_odp_caps; - resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; - } + resp.odp_caps.general_caps = attr.odp_caps.general_caps; + resp.odp_caps.per_transport_caps.rc_odp_caps = + attr.odp_caps.per_transport_caps.rc_odp_caps; + resp.odp_caps.per_transport_caps.uc_odp_caps = + attr.odp_caps.per_transport_caps.uc_odp_caps;Quoted 9 lines
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3335
Quoted 4 lines
drivers/infiniband/core/uverbs_cmd.c +3
-0+ resp.odp_caps.per_transport_caps.ud_odp_caps = + attr.odp_caps.per_transport_caps.ud_odp_caps;#endif + resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ;Not sure about this - if 0 is a valid null answer for all the
_capsthen it is fine, and thecomp_maskbit should just be removed as the size alone should be enough.
Quoted 2 lines
That's difficult to say. But I hope 0 are valids answers in this case.
Hum, according to include/rdma/ib_verbs.h, there's a bit set for ODP support:
148 enum ib_odp_general_cap_bits {
149 IB_ODP_SUPPORT = 1 << 0,
150 };So it should be safe to set everything to 0 in struct ib_uverbs_odp_caps.
Regards.
Signature · 2 lines
Yann Droneaud OPTEYA
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveHi,
Quoted 10 lines · Jason Gunthorpe
Quoted 5 lines · Yann Droneaud
The
write()syscall must return the size buffer passed to it, or less, but in such case it would ask for trouble as userspace would be allowed towrite()the remaining bytes. Returning a size bigger than the one passed towrite()is not acceptable and would break any expectation.By that logic the 0 return is still wrong, and it should be
ucore->in_len
This is handled by ib_uverbs_write() indrivers/infiniband/core/uverbs_main.c:
709 if (err)
710 return err;
711
712 return written_count;Quoted 4 lines
But I think we can return less without risking anything breaking, it already violates the invariant associated with
write()- it mutates the buffer passed in!
I don't think so, as only the response buffer is written to and the response buffer pointer is provided in the buffer given to write().
AFAIK, no uverbs ever change the content of the input buffer (eg. the request): I've managed to declare the various input buffers "const" so it would surprising to find it use for writing to userspace.
Anyway, I recognize that uverb way of abusing write() syscall is borderline (at best) regarding other Linux subsystems and Unix paradigm in general. But it's not enough to screw it more.
Regards.
Signature · 2 lines
Yann Droneaud OPTEYA
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 3 lines · Yann Droneaud
Anyway, I recognize that uverb way of abusing
write()syscall is borderline (at best) regarding other Linux subsystems and Unix paradigm in general. But it's not enough to screw it more.
Then we must return the correct output size explicitly in the struct.
Jason
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveHi,
Quoted 11 lines · Jason Gunthorpe
Quoted 6 lines · Yann Droneaud
As specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's
comp_maskshould describe the request data: it's describe the availability of extended fields in the request. Conversely, the response'scomp_maskshould describe the presence of extended fields in the response.Roland: I agree with Yann, these patches need to go in, or the ODP patches reverted.
Reverting all On Demand Paging patches seems overkill:
if something as to be reverted it should be commit 5a77abf9a97a ("IB/core: Add support for extended query device caps") and the part of commit 860f10a799c8 ("IB/core: Add flags for on demand paging support") which modify ib_uverbs_ex_query_device().
But I wonder about this part of commit 860f10a799c8:
drivers/infiniband/core/uverbs_cmd.c +12 -0
@@ -953,6 +953,18 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file, goto err_free; } +if (cmd.access_flags &IB_ACCESS_ON_DEMAND ) { +struct ib_device_attr attr; + + ret =ib_query_device (pd->device, &attr); +if (ret || !(attr.device_cap_flags & +IB_DEVICE_ON_DEMAND_PAGING )) { +pr_debug ("ODP support not available\n" ); + ret = -EINVAL ; + goto err_put; + } + } +
AFAICT (1 << 6) bit from struct ib_uverbs_reg_mr access_flags field was not enforced to be 0 previously, as ib_check_mr_access() only check for some coherency between a subset of the bits (it's not a function dedicated to check flags provided by userspace):
include/rdma/ib_verbs.h:
1094 enum ib_access_flags {
1095 IB_ACCESS_LOCAL_WRITE = 1, 1096 IB_ACCESS_REMOTE_WRITE = (1<<1), 1097 IB_ACCESS_REMOTE_READ = (1<<2), 1098 IB_ACCESS_REMOTE_ATOMIC = (1<<3), 1099 IB_ACCESS_MW_BIND = (1<<4), 1100 IB_ZERO_BASED = (1<<5), 1101 IB_ACCESS_ON_DEMAND = (1<<6),
1102 };
drivers/infiniband/core/uverbs_cmd.c: ib_uverbs_reg_mr()
961 ret = ib_check_mr_access(cmd.access_flags);
962 if (ret)
963 return ret;include/rdma/ib_verbs.h:
2643 static inline int ib_check_mr_access(int flags) 2644 {
2645 /*
2646 * Local write permission is required if remote write or 2647 * remote atomic permission is also requested. 2648 */
2649 if (flags & (IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_REMOTE_WRITE) &&
2650 !(flags & IB_ACCESS_LOCAL_WRITE))
2651 return -EINVAL;
2652
2653 return 0;
2654 }
drivers/infiniband/core/uverbs_cmd.c: ib_uverbs_reg_mr()
990 mr = pd->device->reg_user_mr(pd, cmd.start, cmd.length, cmd.hca_va,
991 cmd.access_flags, &udata);reg_user_mr() functions may call ib_umem_get() and pass access_flags to it:
drivers/infiniband/core/umem.c: ib_umem_get()
114 /*
115 * We ask for writable memory if any of the following
116 * access flags are set. "Local write" and "remote write"
117 * obviously require write access. "Remote atomic" can do
118 * things like fetch and add, which will modify memory, and
119 * "MW bind" can change permissions by binding a window.
120 */
121 umem->writable = !!(access &
122 (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE |
123 IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_MW_BIND));
124
125 if (access & IB_ACCESS_ON_DEMAND) {
126 ret = ib_umem_odp_get(context, umem);
127 if (ret) {
128 kfree(umem);
129 return ERR_PTR(ret);
130 }
131 return umem;
132 }As you can see only a few bits in access_flags are checked in the end, so it may exist a very unlikely possibility that an existing userspace program is setting this IB_ACCESS_ON_DEMAND bit without the intention of enabling on demand paging as this would be unnoticed by older kernel.
In the other hand, a newer program built with on-demand-paging in mind will set the bit, but when run on older kernel, it will be a no-op, allowing the program to continue, perhaps thinking on-demand-paging is available.
That should be avoided as much as possible.
Unfortunately, I think this cannot be fixed as it's was long since IB_ZERO_BASED was added by commit 7083e42ee2 ("IB/core: Add "type 2" memory windows support").
Anyway there was no check for IB_ACCESS_REMOTE_READ, nor IB_ACCESS_MW_BIND in the uverb layer either.
So, just as the second argument of open() syscall (remember O_TMPFILE, see http://lwn.net/Articles/562294/ ), we will have to live with and be careful ...
Regards.
Signature · 2 lines
Yann Droneaud OPTEYA
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 8 lines · Yann Droneaud
Quoted 2 lines
Roland: I agree with Yann, these patches need to go in, or the ODP patches reverted.
Reverting all On Demand Paging patches seems overkill:
if something as to be reverted it should be commit5a77abf9a97a("IB/core: Add support for extended query device caps") and the part of commit860f10a799c8("IB/core: Add flags for on demand paging support") which modifyib_uverbs_ex_query_device().
Thank you and Jason for taking on this interface.
At this point I feel like I do about the IPoIB changes -- we should revert the broken stuff and get it right for 3.20.
If we revert the two things you describe above, is everything else OK to leave in 3.19 with respect to ABI?
Thanks,
Roland
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveHi,
Quoted 10 lines · Roland Dreier
Quoted 8 lines · Yann Droneaud
Quoted 2 lines
Roland: I agree with Yann, these patches need to go in, or the ODP patches reverted.
Reverting all On Demand Paging patches seems overkill:
if something as to be reverted it should be commit5a77abf9a97a("IB/core: Add support for extended query device caps") and the part of commit860f10a799c8("IB/core: Add flags for on demand paging support") which modifyib_uverbs_ex_query_device().
Quoted 8 lines
Thank you and Jason for taking on this interface.
At this point I feel like I do about the IPoIB changes -- we should revert the broken stuff and get it right for 3.20.
If we revert the two things you describe above, is everything else OK to leave in 3.19 with respect to ABI?
I've tried to review every changes since v3.18 on drivers/infiniband include/rdma and include/uapi/rdma with respect to ABI issues.
I've noticed no other issue, but I have to admit I've not well reviewed the drivers (hw/) internal changes.
If the IB_USER_VERBS_EX_CMD_QUERY_DEVICE and ib_uverbs_ex_query_device changes are going to be reverted for v3.19, the on-demand-paging feature will be available (IB_DEVICE_ON_DEMAND_PAGING will be set device_cap_flags in response to non extended QUERY_DEVICE for mlx5 HCA and IB_ACCESS_ON_DEMAND access flag will be effective for REG_MR uverbs), but its parameters won't be. I don't know if it's a no-go for the usage of on-demand paging by userspace: I have not the chance of owning HCA with the support for this feature, nor the patches libibverbs / libmlx5 ... (anyway I would not have the time to test). I've hoped people from Mellanox would have commented on the revert option too.
Regards.
Signature · 2 lines
Yann Droneaud OPTEYA
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 3 lines · Yann Droneaud
[..] I have not the chance
of owning HCA with the support for this feature, nor the patches libibverbs / libmlx5 ... (anyway I would not have the time to test). [...]
Yann, have you ever run Linux over RDMA capable HW (IB, RoCE, iWARP or alike?)
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Yann Droneaud
Quoted 13 lines
drivers/infiniband/core/uverbs_cmd.c +4
-9- resp.odp_caps.general_caps = attr.odp_caps.general_caps; - resp.odp_caps.per_transport_caps.rc_odp_caps = - attr.odp_caps.per_transport_caps.rc_odp_caps; - resp.odp_caps.per_transport_caps.uc_odp_caps = - attr.odp_caps.per_transport_caps.uc_odp_caps; - resp.odp_caps.per_transport_caps.ud_odp_caps = - attr.odp_caps.per_transport_caps.ud_odp_caps; - resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; - } + resp.odp_caps.general_caps = attr.odp_caps.general_caps; + resp.odp_caps.per_transport_caps.rc_odp_caps = + attr.odp_caps.per_transport_caps.rc_odp_caps; + resp.odp_caps.per_transport_caps.uc_odp_caps =
Quoted 8 lines
Quoted 5 lines
drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3335
Quoted 5 lines
drivers/infiniband/core/uverbs_cmd.c +4
-0+ attr.odp_caps.per_transport_caps.uc_odp_caps; + resp.odp_caps.per_transport_caps.ud_odp_caps = + attr.odp_caps.per_transport_caps.ud_odp_caps;#endif + resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ;Not sure about this - if 0 is a valid null answer for all the
_capsthen it is fine, and thecomp_maskbit should just be removed as the size alone should be enough.
Zero is indeed a valid answer. There the IB_ODP_SUPPORT bit in the general_caps field that says whether or not ODP is supported in general. The per transport capabilities are also default to not supported.
However, I think we should keep the comp_mask field for future extensions. The current code doesn't report the size of the response to user space, and in addition, comp_mask being a bit mask has the advantage of allowing only part of the structure to be marked valid.
Regards,
Haggai
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 97 lines · Yann Droneaud
But I wonder about this part of commit
860f10a799c8:drivers/infiniband/core/uverbs_cmd.c +12
-0@@ -953,6 +953,18 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file, goto err_free; } +if (cmd.access_flags &IB_ACCESS_ON_DEMAND ) { +struct ib_device_attr attr; + + ret =ib_query_device (pd->device, &attr); +if (ret || !(attr.device_cap_flags & +IB_DEVICE_ON_DEMAND_PAGING )) { +pr_debug ("ODP support not available\n" ); + ret = -EINVAL ; + goto err_put; + } + } +AFAICT (1 << 6) bit from struct
ib_uverbs_reg_mraccess_flagsfield was not enforced to be 0 previously, asib_check_mr_access()only check for some coherency between a subset of the bits (it's not a function dedicated to check flags provided by userspace):
include/rdma/ib_verbs.h:1094 enum ib_access_flags { 1095 IB_ACCESS_LOCAL_WRITE = 1, 1096 IB_ACCESS_REMOTE_WRITE = (1<<1), 1097 IB_ACCESS_REMOTE_READ = (1<<2), 1098 IB_ACCESS_REMOTE_ATOMIC = (1<<3), 1099 IB_ACCESS_MW_BIND = (1<<4), 1100 IB_ZERO_BASED = (1<<5), 1101 IB_ACCESS_ON_DEMAND = (1<<6), 1102 };
drivers/infiniband/core/uverbs_cmd.c:ib_uverbs_reg_mr()961 ret = ib_check_mr_access(cmd.access_flags); 962 if (ret) 963 return ret;
include/rdma/ib_verbs.h:2643 static inline int ib_check_mr_access(int flags) 2644 { 2645 /* 2646 * Local write permission is required if remote write or 2647 * remote atomic permission is also requested. 2648 */ 2649 if (flags & (IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_REMOTE_WRITE) && 2650 !(flags & IB_ACCESS_LOCAL_WRITE)) 2651 return -EINVAL; 2652 2653 return 0; 2654 }
drivers/infiniband/core/uverbs_cmd.c:ib_uverbs_reg_mr()990 mr = pd->device->reg_user_mr(pd, cmd.start, cmd.length, cmd.hca_va, 991 cmd.access_flags, &udata);
reg_user_mr()functions may callib_umem_get()and passaccess_flagsto it:
drivers/infiniband/core/umem.c:ib_umem_get()114 /* 115 * We ask for writable memory if any of the following 116 * access flags are set. "Local write" and "remote write" 117 * obviously require write access. "Remote atomic" can do 118 * things like fetch and add, which will modify memory, and 119 * "MW bind" can change permissions by binding a window. 120 */ 121 umem->writable = !!(access & 122 (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE | 123 IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_MW_BIND)); 124 125 if (access & IB_ACCESS_ON_DEMAND) { 126 ret = ib_umem_odp_get(context, umem); 127 if (ret) { 128 kfree(umem); 129 return ERR_PTR(ret); 130 } 131 return umem; 132 }As you can see only a few bits in
access_flagsare checked in the end, so it may exist a very unlikely possibility that an existing userspace program is setting thisIB_ACCESS_ON_DEMANDbit without the intention of enabling on demand paging as this would be unnoticed by older kernel.
Quoted 4 lines
In the other hand, a newer program built with on-demand-paging in mind will set the bit, but when run on older kernel, it will be a no-op, allowing the program to continue, perhaps thinking on-demand-paging is available.
This is why we added a method for userspace to check the kernel capabilities both when adding the memory windows support and with ODP. User-space can still send garbage to the kernel, but if it does so without checking the kernel supports its request, it's user-space's problem.
Quoted 7 lines
That should be avoided as much as possible.
Unfortunately, I think this cannot be fixed as it's was long since
IB_ZERO_BASEDwas added by commit7083e42ee2("IB/core: Add "type 2" memory windows support").
Anyway there was no check forIB_ACCESS_REMOTE_READ, norIB_ACCESS_MW_BINDin the uverb layer either.
I think it would be perfectly fine to add a check today that validates the MR access flags input is known. Because the newer feature require user-space to check capabilities, I don't think it would matter if we returned an error on newer kernels that do not support these features.
Haggai
Haggai Eran
Reviewed-byRe: [PATCH v1 2/5] IB/uverbs: ex_query_device: check request's comp_mask
In reply to a message not in the archiveQuoted 10 lines · Yann Droneaud
This patch ensures the extended
QUERY_DEVICEuverbs request'scomp_maskhas only known and supported bits (currently none).If userspace set unknown features bits,
-EINVALwill be returned, ensuring current programs are not allowed to set random feature
bits: such bits could enable new extended features in future kernel versions and those features can trigger a behavior not unsupported by the older programs or make the newer kernels return an error for a request which was valid on older kernels.
Quoted 3 lines
Additionally, returning an error for unsupported feature would allow userspace to probe/discover which extended features are currently supported by a kernel.
As I wrote before, I hope in the future we don't force userspace to probe features this way, because it may be unnecessarily complex.
I agree though that we should have a way to extend this verb in the future.
Quoted 26 lines
Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]>Signed-off-by Yann Droneaud <[email protected]>--- drivers/infiniband/core/uverbs_cmd.c | 3 +++ 1 file changed, 3 insertions(+)drivers/infiniband/core/uverbs_cmd.c +3
-0@@ -3312,6 +3312,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,if (err)return err; +if (cmd.comp_mask) +return -EINVAL ; +if (cmd.reserved)return -EINVAL ;
Haggai Eran
Reviewed-byRe: [PATCH v1 3/5] IB/uverbs: ex_query_device: answer must depend on response buffer's size
In reply to a message not in the archiveQuoted 39 lines · Yann Droneaud
As specified in "Extending Verbs API" presentation [1] by Tzahi Oved during OFA International Developer Workshop 2013, the request's
comp_maskshould describe the request data: it's describe the availability of extended fields in the request. Conversely, the response'scomp_maskshould describe the presence of extended fields in the response.So instead of silently truncating extended
QUERY_DEVICEuverb's response, see commit5a77abf9a97a("IB/core: Add support for extended query device caps")), this patch makes functionib_uverbs_ex_query_device()check the available space in the response buffer against the minimal response structure and fail with-ENOSPCif this base structure cannot be returned to
userspace: it's required to be able to return the comp_mask's value, at least.For extended features, currently only
IB_USER_VERBS_EX_QUERY_DEVICE_ODPper commit860f10a799c8("IB/core: Add flags for on demand paging support"), the extension's data is returned if the needed space is available in the response buffer: it is expected that newer userspace program pass a bigger response buffer so that it can retrieve extended features. Thecomp_maskvalue will match the fields that were effectively returned to userspace.In the end:
- userspace won't get truncated responses;
- newer kernel would be able to support older binaries and older binaries will work flawlessly with newer kernel;
- additionally, newer binaries would even be able to support older kernel, as far as they don't set new feature bit in request'scomp_mask.Note: as
offsetof()is used to retrieve the size of the lower chunk of the response, beware that it only works if the upper chunk is right after, without any implicit padding. And, as the size of the latter chunk is added to the base size, implicit padding at the end of the structure is not taken in account. Both point must be taken in account when extending the uverbs functionalities.
Quoted 7 lines
Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]>
Quoted 52 lines
Signed-off-by Yann Droneaud <[email protected]>--- drivers/infiniband/core/uverbs_cmd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)drivers/infiniband/core/uverbs_cmd.c +12
-1@@ -3302,6 +3302,7 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,struct ib_uverbs_ex_query_device cmd;struct ib_device_attr attr;struct ib_device *device; +size_t resp_len;int err; device = file->device->ib_dev;@@ -3318,6 +3319,11 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,if (cmd.reserved)return -EINVAL ; + resp_len = offsetof(typeof (resp ), odp_caps); + +if (ucore->outlen < resp_len) +return -ENOSPC ; + err = device->query_device (device, &attr);if (err)return err;@@ -3326,6 +3332,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,copy_query_dev_fields (file, &resp.base, &attr); resp.comp_mask =0 ; +if (ucore->outlen < resp_len +sizeof (resp.odp_caps)) + goto end; +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING resp.odp_caps.general_caps = attr.odp_caps.general_caps; resp.odp_caps.per_transport_caps.rc_odp_caps =@@ -3336,8 +3345,10 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, attr.odp_caps.per_transport_caps.ud_odp_caps;#endif resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; + resp_len +=sizeof (resp.odp_caps); - err =ib_copy_to_udata (ucore, &resp,sizeof (resp)); +end: + err =ib_copy_to_udata (ucore, &resp, resp_len);if (err)return err;
Haggai Eran
Reviewed-byRe: [PATCH v1 4/5] IB/uverbs: ex_query_device: no need to clear the whole structure
In reply to a message not in the archiveQuoted 3 lines · Yann Droneaud
As only the requested fields are set and copied to userspace, there's no need to clear the content of the response structure beforehand.
Care must be taken to zero out all the data that is copied to userspace, but I agree it can be done only to the parts that are unsupported as this patch does.
Quoted 36 lines
Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]>Signed-off-by Yann Droneaud <[email protected]>--- drivers/infiniband/core/uverbs_cmd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/infiniband/core/uverbs_cmd.c +4
-1@@ -3328,9 +3328,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,if (err)return err; -memset (&resp,0 ,sizeof (resp));copy_query_dev_fields (file, &resp.base, &attr); resp.comp_mask =0 ; + resp.reserved =0 ;if (ucore->outlen < resp_len +sizeof (resp.odp_caps)) goto end;@@ -3343,6 +3343,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, attr.odp_caps.per_transport_caps.uc_odp_caps; resp.odp_caps.per_transport_caps.ud_odp_caps = attr.odp_caps.per_transport_caps.ud_odp_caps; + resp.odp_caps.reserved =0 ; +#else +memset (&resp.odp_caps,0 ,sizeof (resp.odp_caps));#endif resp.comp_mask |=IB_USER_VERBS_EX_QUERY_DEVICE_ODP ; resp_len +=sizeof (resp.odp_caps);
Haggai Eran
Reviewed-byRe: [PATCH v1 5/5] IB/core: ib_copy_to_udata(): don't silently truncate response
In reply to a message not in the archiveQuoted 28 lines · Yann Droneaud
While
ib_copy_to_udata()should check for the available output space as already proposed in some other patches [1][2][3], the changes brought by commit5a77abf9a97a("IB/core: Add support for extended query device caps") are silently truncating the data to be written to userspace if the output buffer is not large enough to hold the response data.Silently truncating the response is not a reliable behavior as userspace is not given any hint about this truncation: userspace is leaved with garbage to play with.
Not checking the response buffer size and writing past the userspace buffer is no good either, but it's the current behavior.
So this patch revert the particular change on
ib_copy_to_udata()as a better behavior is implemented in the upper level functionib_uverbs_ex_query_device().[1] "[PATCH 00/22] infiniband: improve userspace input check"
http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[2] "[PATCH 03/22] infiniband:
ib_copy_from_udata(): check input length"[3] "[PATCH 04/22] infiniband:
ib_copy_to_udata(): check output length"
Quoted 7 lines
Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]>
Quoted 22 lines
Signed-off-by Yann Droneaud <[email protected]>--- include/rdma/ib_verbs.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)include/rdma/ib_verbs.h +1
-4@@ -1707,10 +1707,7 @@ static inline int ib_copy_from_udata(void *dest, struct ib_udata *udata, size_tstatic inline int ib_copy_to_udata (struct ib_udata *udata,void *src,size_t len) { -size_t copy_sz; - - copy_sz =min_t (size_t, len, udata->outlen); -return copy_to_user (udata->outbuf, src, copy_sz) ? -EFAULT :0 ; +return copy_to_user (udata->outbuf, src, len) ? -EFAULT :0 ; }/**
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 28 lines · Yann Droneaud
Hi,
Quoted 10 lines · Roland Dreier
Quoted 8 lines · Yann Droneaud
Quoted 2 lines
Roland: I agree with Yann, these patches need to go in, or the ODP patches reverted.
Reverting all On Demand Paging patches seems overkill:
if something as to be reverted it should be commit5a77abf9a97a("IB/core: Add support for extended query device caps") and the part of commit860f10a799c8("IB/core: Add flags for on demand paging support") which modifyib_uverbs_ex_query_device().Quoted 8 lines
Thank you and Jason for taking on this interface.
At this point I feel like I do about the IPoIB changes -- we should revert the broken stuff and get it right for 3.20.
If we revert the two things you describe above, is everything else OK to leave in 3.19 with respect to ABI?
I've tried to review every changes since v3.18 on
drivers/infinibandinclude/rdmaandinclude/uapi/rdmawith respect to ABI issues.I've noticed no other issue, but I have to admit I've not well reviewed the drivers (hw/) internal changes.
Quoted 6 lines
If the
IB_USER_VERBS_EX_CMD_QUERY_DEVICEandib_uverbs_ex_query_devicechanges are going to be reverted for v3.19, the on-demand-paging feature will be available (IB_DEVICE_ON_DEMAND_PAGINGwill be setdevice_cap_flagsin response to non extendedQUERY_DEVICEfor mlx5 HCA andIB_ACCESS_ON_DEMANDaccess flag will be effective forREG_MRuverbs), but its parameters won't be.
For user-space to make use of on demand paging, it should verify the specific transport and operation is supported. If they don't, they will encounter errors when a page fault occurs.
Quoted 6 lines
I don't know if it's a no-go for
the usage of on-demand paging by userspace: I have not the chance of owning HCA with the support for this feature, nor the patches libibverbs / libmlx5 ... (anyway I would not have the time to test). I've hoped people from Mellanox would have commented on the revert option too.
I would prefer it if Yann's patches are accepted. I understand it is very late, but they are quite short, and I think they provide the right semantic for this new verb.
As a second option, in case you prefer to revert the extended query device patch, I will send a patch shortly to do that.
Regards,
Haggai
Re: [PATCH v1 2/5] IB/uverbs: ex_query_device: check request's comp_mask
In reply to a message not in the archiveHi,
Quoted 11 lines · Haggai Eran
Quoted 10 lines · Yann Droneaud
This patch ensures the extended
QUERY_DEVICEuverbs request'scomp_maskhas only known and supported bits (currently none).If userspace set unknown features bits,
-EINVALwill be returned, ensuring current programs are not allowed to set random feature
bits: such bits could enable new extended features in future kernel versions and those features can trigger a behavior not unsupported by the older programs or make the newer kernels return an error for a request which was valid on older kernels.
Quoted 7 lines
Quoted 3 lines
Additionally, returning an error for unsupported feature would allow userspace to probe/discover which extended features are currently supported by a kernel.
As I wrote before, I hope in the future we don't force userspace to probe features this way, because it may be unnecessarily complex.
I believe that most use cases won't need probing as applications are often built according to the current kernel features in mind.
If applications need to use new features, it seems to be a small price to pay to be prepared to get -EINVAL.
In another word: backward compatibility from application point of view:
a newer application wanting to run on older kernel must be prepared to.
Quoted 4 lines
I agree though that we should have a way to extend this verb in the future.
Thanks.
Quoted 31 lines
Quoted 17 lines
Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/[email protected]>Signed-off-by Yann Droneaud <[email protected]>--- drivers/infiniband/core/uverbs_cmd.c | 3 +++ 1 file changed, 3 insertions(+)drivers/infiniband/core/uverbs_cmd.c +0
-0drivers/infiniband/core/uverbs_cmd.c int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, · line 3320
Quoted 9 lines
drivers/infiniband/core/uverbs_cmd.c +3
-0@@ -3312,6 +3312,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,if (err)return err; +if (cmd.comp_mask) +return -EINVAL ; +if (cmd.reserved)return -EINVAL ;--
To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 8 lines · Yann Droneaud
Quoted 4 lines
Anyway, I recognize that uverb way of abusing
write()syscall is borderline (at best) regarding other Linux subsystems and Unix paradigm in general. But it's not enough to screw it more.Then we must return the correct output size explicitly in the struct.
I was thinking this very same thing as I read through this thread.
I too would like to avoid the use of comp_masks if at all possible. The query call seems to be a verb where the structure size is all you really need to know the set of values returned.
As Jason says, other calls may require the comp_mask where 0 is not sufficient to indicate "missing".
Ira
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 7 lines · Ira
Quoted 7 lines · Yann Droneaud
Quoted 4 lines
Anyway, I recognize that uverb way of abusing
write()syscall is borderline (at best) regarding other Linux subsystems and Unix paradigm in general. But it's not enough to screw it more.
Quoted 7 lines
Quoted 1 line
Then we must return the correct output size explicitly in the struct.
I was thinking this very same thing as I read through this thread.
I too would like to avoid the use of
comp_masksif at all possible. The query call seems to be a verb where the structure size is all you really need to know the set of values returned.As Jason says, other calls may require the
comp_maskwhere 0 is not sufficient to indicate "missing".
Would it be okay to return it in the ib_uverbs_cmd_hdr.out_words? That would further abuse the write() syscall by writing to the input buffer. However, the only other alternative I see is to add it explicitly to every uverb response struct.
Haggai
RE: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 14 lines · Ira
Quoted 7 lines
Quoted 7 lines · Yann Droneaud
Quoted 4 lines
Anyway, I recognize that uverb way of abusing
write()syscall is borderline (at best) regarding other Linux subsystems and Unix paradigm in general. But it's not enough to screw it more.Quoted 5 lines
Quoted 1 line
Then we must return the correct output size explicitly in the struct.
I was thinking this very same thing as I read through this thread.
I too would like to avoid the use of
comp_masksif at all possible. The query
Quoted 8 lines
call seems to be a verb where the structure size is all you really need to know the set of values returned.
Quoted 2 lines
As Jason says, other calls may require the
comp_maskwhere 0 is notsufficient to indicate "missing".
Would it be okay to return it in the ib_uverbs_cmd_hdr.out_words? That would further abuse the
write()syscall by writing to the input buffer.
I don't think that is such a great idea.
Quoted 3 lines
However, the only other alternative I see is to add it explicitly to every uverb response struct.
I think this is the best solution. There is a 32 bit reserved field in ib_uverbs_ex_query_device_resp. Could we use all or part of that to be the size?
For other extended commands I'm not sure what to do.
Ira
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveQuoted 20 lines · Ira
Quoted 20 lines · Ira
Quoted 7 lines
Quoted 7 lines · Yann Droneaud
Quoted 4 lines
Anyway, I recognize that uverb way of abusing
write()syscall is borderline (at best) regarding other Linux subsystems and Unix paradigm in general. But it's not enough to screw it more.Quoted 5 lines
Quoted 1 line
Then we must return the correct output size explicitly in the struct.
I was thinking this very same thing as I read through this thread.
I too would like to avoid the use of
comp_masksif at all possible. The querycall seems to be a verb where the structure size is all you really need to know the set of values returned.
Quoted 2 lines
As Jason says, other calls may require the
comp_maskwhere 0 is notsufficient to indicate "missing".
...
Quoted 6 lines
Quoted 4 lines
However, the only other alternative I see is to add it explicitly to every uverb response struct.
I think this is the best solution. There is a 32 bit reserved field in
ib_uverbs_ex_query_device_resp. Could we use all or part of that to be the size?
Yes, I think 32-bit for the response length are more than enough.
I will send patches for 3.20 to re-introduce ib_uverbs_ex_query_device with the response size instead of the reserved field, and with Yann's changes.
Thanks,
Haggai
Re: [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
In reply to a message not in the archiveHi Or,
Quoted 6 lines · Or Gerlitz
Quoted 3 lines · Yann Droneaud
[..] I have not the chance
of owning HCA with the support for this feature, nor the patches libibverbs / libmlx5 ... (anyway I would not have the time to test). [...]Yann, have you ever run Linux over RDMA capable HW (IB, RoCE, iWARP or alike?)
That was a silly question.
So I've took a little time to consider answering it, perhaps you were gently kidding me.
Anyway, please take the following links as an answer to your question:
"rdma_create_qp() and max_send_wr"
<http://mid.gmane.org/1303404264.2243.19.camel-H/[email protected]>
"ibv_create_qp() and max_inline_data behavior"
<http://mid.gmane.org/[email protected]>
"[PATCH librdmacm 0/3] no IBV_SEND_INLINE thus memory registration required on QLogic/Intel HCA"
<http://mid.gmane.org/cover.1376746185.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
"[PATCH 00/22] infiniband: improve userspace input check"
<http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
I hope this would be enough to join the gang^Wclub. Where're the by-laws ?
Regards.
Signature · 2 lines
Yann Droneaud OPTEYA