| Message ID | d60715123c640bc7b720ad11a9faa3af78950aa6.1421931555.git.ydroneaud@opteya.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 22/01/2015 15:28, Yann Droneaud wrote: > 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 requested features set in comp_mask > (currently only IB_USER_VERBS_EX_QUERY_DEVICE_ODP per commit > 860f10a799c8 ("IB/core: Add flags for on demand paging support"). Currently you only need to check two possible sizes. With each additional field you will need to check another size. Sooner or later someone will forget to add a check and will create a non-backward compatible kernel. > If the response buffer is not large enough to store the expected > response, -ENOSPC is returned to userspace so that it can adjust > the size of its buffer. > > 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. Another point future contributors will easily miss. > Link: http://mid.gmane.org/cover.1421931555.git.ydroneaud@opteya.com > Cc: Sagi Grimberg <sagig@mellanox.com> > Cc: Shachar Raindel <raindel@mellanox.com> > Cc: Eli Cohen <eli@mellanox.com> > Cc: Haggai Eran <haggaie@mellanox.com> > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> > --- > drivers/infiniband/core/uverbs_cmd.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > index 532d8eba8b02..8668b328b7e6 100644 > --- a/drivers/infiniband/core/uverbs_cmd.c > +++ b/drivers/infiniband/core/uverbs_cmd.c > @@ -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; > @@ -3315,6 +3316,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; > @@ -3325,6 +3331,11 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, > > #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > if (cmd.comp_mask & IB_USER_VERBS_EX_QUERY_DEVICE_ODP) { > + resp_len += sizeof(resp.odp_caps); > + > + if (ucore->outlen < resp_len) > + return -ENOSPC; > + > 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; > @@ -3336,7 +3347,7 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, > } > #endif > > - err = ib_copy_to_udata(ucore, &resp, sizeof(resp)); > + err = ib_copy_to_udata(ucore, &resp, resp_len); > if (err) > return err; > > -- 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
Hi, Le dimanche 25 janvier 2015 à 17:29 +0200, Haggai Eran a écrit : > On 22/01/2015 15:28, Yann Droneaud wrote: > > 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 requested features set in comp_mask > > (currently only IB_USER_VERBS_EX_QUERY_DEVICE_ODP per commit > > 860f10a799c8 ("IB/core: Add flags for on demand paging support"). > > Currently you only need to check two possible sizes. With each > additional field you will need to check another size. Yes, that's mandatory for any interface with a dynamic, ever growing, data structure. > Sooner or later someone will forget to add a check and will create a non-backward > compatible kernel. > That's why we have to review userspace interface patch with lot of care. Anyway, such a change should be fixed easily as it's would be definitively a bug. If the behavior I'm proposing is in place, userspace won't be allowed to supply unsupported bits in a request, so older userspace requests on newer kernels won't trigger unsupported new behavior. This ensure we can extend the interfance with no risk of breaking existing program. > > If the response buffer is not large enough to store the expected > > response, -ENOSPC is returned to userspace so that it can adjust > > the size of its buffer. > > > > 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. > > Another point future contributors will easily miss. > One should use "pahole" tool when modifying data structure part of the ABI to ensure the data structure are correctly aligned on various architectures. > > Link: http://mid.gmane.org/cover.1421931555.git.ydroneaud@opteya.com > > Cc: Sagi Grimberg <sagig@mellanox.com> > > Cc: Shachar Raindel <raindel@mellanox.com> > > Cc: Eli Cohen <eli@mellanox.com> > > Cc: Haggai Eran <haggaie@mellanox.com> > > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> > > --- > > drivers/infiniband/core/uverbs_cmd.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > > index 532d8eba8b02..8668b328b7e6 100644 > > --- a/drivers/infiniband/core/uverbs_cmd.c > > +++ b/drivers/infiniband/core/uverbs_cmd.c > > @@ -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; > > @@ -3315,6 +3316,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; > > @@ -3325,6 +3331,11 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, > > > > #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > > if (cmd.comp_mask & IB_USER_VERBS_EX_QUERY_DEVICE_ODP) { > > + resp_len += sizeof(resp.odp_caps); > > + > > + if (ucore->outlen < resp_len) > > + return -ENOSPC; > > + > > 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; > > @@ -3336,7 +3347,7 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, > > } > > #endif > > > > - err = ib_copy_to_udata(ucore, &resp, sizeof(resp)); > > + err = ib_copy_to_udata(ucore, &resp, resp_len); > > if (err) > > return err; > > > > > Regards.
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index 532d8eba8b02..8668b328b7e6 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -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; @@ -3315,6 +3316,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; @@ -3325,6 +3331,11 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING if (cmd.comp_mask & IB_USER_VERBS_EX_QUERY_DEVICE_ODP) { + resp_len += sizeof(resp.odp_caps); + + if (ucore->outlen < resp_len) + return -ENOSPC; + 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; @@ -3336,7 +3347,7 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file, } #endif - err = ib_copy_to_udata(ucore, &resp, sizeof(resp)); + err = ib_copy_to_udata(ucore, &resp, resp_len); if (err) return err;
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 requested features set in comp_mask (currently only IB_USER_VERBS_EX_QUERY_DEVICE_ODP per commit 860f10a799c8 ("IB/core: Add flags for on demand paging support"). If the response buffer is not large enough to store the expected response, -ENOSPC is returned to userspace so that it can adjust the size of its buffer. 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. Link: http://mid.gmane.org/cover.1421931555.git.ydroneaud@opteya.com Cc: Sagi Grimberg <sagig@mellanox.com> Cc: Shachar Raindel <raindel@mellanox.com> Cc: Eli Cohen <eli@mellanox.com> Cc: Haggai Eran <haggaie@mellanox.com> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> --- drivers/infiniband/core/uverbs_cmd.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)