| Message ID | 335da45738872e446f63db338ca766a34608c90a.1422553023.git.ydroneaud@opteya.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jan 29, 2015 at 06:59:59PM +0100, Yann Droneaud wrote: > This patch ensures the extended QUERY_DEVICE uverbs request's > comp_mask has 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. Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Le jeudi 29 janvier 2015 à 11:36 -0700, Jason Gunthorpe a écrit : > On Thu, Jan 29, 2015 at 06:59:59PM +0100, Yann Droneaud wrote: > > This patch ensures the extended QUERY_DEVICE uverbs request's > > comp_mask has 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. > 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. > Reviewed-By: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Thanks. > 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. > 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.
On Thu, Jan 29, 2015 at 11:36:48AM -0700, Jason Gunthorpe wrote: > 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. Never mind this, I see the copy_query_dev_fields call now! Thanks, Jason -- 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
On 29/01/2015 19:59, Yann Droneaud wrote: > This 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. 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. Reviewed-by: Haggai Eran <haggaie@mellanox.com> > > Link: http://mid.gmane.org/cover.1422553023.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 | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > index 6ef06a9b4362..fbcc54b86795 100644 > --- a/drivers/infiniband/core/uverbs_cmd.c > +++ b/drivers/infiniband/core/uverbs_cmd.c > @@ -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-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 01 février 2015 à 10:12 +0200, Haggai Eran a écrit : > On 29/01/2015 19:59, Yann Droneaud wrote: > > This 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. > > 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. > I agree though that we should have a way to extend this verb in the future. > > Reviewed-by: Haggai Eran <haggaie@mellanox.com> > Thanks. > > > > Link: http://mid.gmane.org/cover.1422553023.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 | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c > > index 6ef06a9b4362..fbcc54b86795 100644 > > --- a/drivers/infiniband/core/uverbs_cmd.c > > +++ b/drivers/infiniband/core/uverbs_cmd.c > > @@ -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 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
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index 6ef06a9b4362..fbcc54b86795 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -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;
This 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. Link: http://mid.gmane.org/cover.1422553023.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 | 3 +++ 1 file changed, 3 insertions(+)