| Message ID | 1556613999-14823-1-git-send-email-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f89adedaf3feb2e1a896b2f2387cdcb4e2b9c48b |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next] RDMA/uverbs: Initialize udata struct on destroy flows | expand |
On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: > Cited commit introduced the udata parameter to different destroy flows > but the uapi method definition does not have udata (i.e has_udata flag > is not set). As a result, an uninitialized udata struct is being passed > down to the driver callbacks. > > Fix that by clearing the driver udata even in cases where has_udata flag > is not set. > > Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") > Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> What is wrong with Signed-off-by that caused you to add new tag? Thanks > Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > drivers/infiniband/core/uverbs_ioctl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c > index cfbef25b3a73..829b0c6944d8 100644 > --- a/drivers/infiniband/core/uverbs_ioctl.c > +++ b/drivers/infiniband/core/uverbs_ioctl.c > @@ -453,6 +453,8 @@ static int ib_uverbs_run_method(struct bundle_priv *pbundle, > uverbs_fill_udata(&pbundle->bundle, > &pbundle->bundle.driver_udata, > UVERBS_ATTR_UHW_IN, UVERBS_ATTR_UHW_OUT); > + else > + pbundle->bundle.driver_udata = (struct ib_udata){}; > > if (destroy_bkey != UVERBS_API_ATTR_BKEY_LEN) { > struct uverbs_obj_attr *destroy_attr = > -- > 2.7.4 >
On 30-Apr-19 14:18, Leon Romanovsky wrote: > On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: >> Cited commit introduced the udata parameter to different destroy flows >> but the uapi method definition does not have udata (i.e has_udata flag >> is not set). As a result, an uninitialized udata struct is being passed >> down to the driver callbacks. >> >> Fix that by clearing the driver udata even in cases where has_udata flag >> is not set. >> >> Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") >> Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> >> Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> > > What is wrong with Signed-off-by that caused you to add new tag? Jason is the one that originally wrote and sent the code, this tag seems appropriate. Obviously I don't mind removing it, it's there to give him credit.. > >> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca> >> Signed-off-by: Gal Pressman <galpress@amazon.com> >> --- >> drivers/infiniband/core/uverbs_ioctl.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c >> index cfbef25b3a73..829b0c6944d8 100644 >> --- a/drivers/infiniband/core/uverbs_ioctl.c >> +++ b/drivers/infiniband/core/uverbs_ioctl.c >> @@ -453,6 +453,8 @@ static int ib_uverbs_run_method(struct bundle_priv *pbundle, >> uverbs_fill_udata(&pbundle->bundle, >> &pbundle->bundle.driver_udata, >> UVERBS_ATTR_UHW_IN, UVERBS_ATTR_UHW_OUT); >> + else >> + pbundle->bundle.driver_udata = (struct ib_udata){}; >> >> if (destroy_bkey != UVERBS_API_ATTR_BKEY_LEN) { >> struct uverbs_obj_attr *destroy_attr = >> -- >> 2.7.4 >>
On 4/30/2019 7:27 AM, Gal Pressman wrote: > On 30-Apr-19 14:18, Leon Romanovsky wrote: >> On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: >>> Cited commit introduced the udata parameter to different destroy flows >>> but the uapi method definition does not have udata (i.e has_udata flag >>> is not set). As a result, an uninitialized udata struct is being passed >>> down to the driver callbacks. >>> >>> Fix that by clearing the driver udata even in cases where has_udata flag >>> is not set. >>> >>> Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") >>> Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> >>> Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> >> >> What is wrong with Signed-off-by that caused you to add new tag? > > Jason is the one that originally wrote and sent the code, this tag seems > appropriate. > Obviously I don't mind removing it, it's there to give him credit.. Did you find documentation for using that tag or did you just make it up? I think Signed-off-by is what you want here. -Denny
On 30-Apr-19 14:35, Dennis Dalessandro wrote: > On 4/30/2019 7:27 AM, Gal Pressman wrote: >> On 30-Apr-19 14:18, Leon Romanovsky wrote: >>> On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: >>>> Cited commit introduced the udata parameter to different destroy flows >>>> but the uapi method definition does not have udata (i.e has_udata flag >>>> is not set). As a result, an uninitialized udata struct is being passed >>>> down to the driver callbacks. >>>> >>>> Fix that by clearing the driver udata even in cases where has_udata flag >>>> is not set. >>>> >>>> Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") >>>> Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> >>>> Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> >>> >>> What is wrong with Signed-off-by that caused you to add new tag? >> >> Jason is the one that originally wrote and sent the code, this tag seems >> appropriate. >> Obviously I don't mind removing it, it's there to give him credit.. > > Did you find documentation for using that tag or did you just make it up? I > think Signed-off-by is what you want here. https://www.kernel.org/doc/html/v5.0/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
On Tue, Apr 30, 2019 at 02:38:30PM +0300, Gal Pressman wrote: > On 30-Apr-19 14:35, Dennis Dalessandro wrote: > > On 4/30/2019 7:27 AM, Gal Pressman wrote: > >> On 30-Apr-19 14:18, Leon Romanovsky wrote: > >>> On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: > >>>> Cited commit introduced the udata parameter to different destroy flows > >>>> but the uapi method definition does not have udata (i.e has_udata flag > >>>> is not set). As a result, an uninitialized udata struct is being passed > >>>> down to the driver callbacks. > >>>> > >>>> Fix that by clearing the driver udata even in cases where has_udata flag > >>>> is not set. > >>>> > >>>> Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") > >>>> Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > >>>> Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> > >>> > >>> What is wrong with Signed-off-by that caused you to add new tag? > >> > >> Jason is the one that originally wrote and sent the code, this tag seems > >> appropriate. > >> Obviously I don't mind removing it, it's there to give him credit.. > > > > Did you find documentation for using that tag or did you just make it up? I > > think Signed-off-by is what you want here. > > https://www.kernel.org/doc/html/v5.0/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by I see no benefit of this new tag over SOB, especially for the patch which has 100% code of that Co-* author. Thanks
On 30-Apr-19 15:07, Leon Romanovsky wrote: > On Tue, Apr 30, 2019 at 02:38:30PM +0300, Gal Pressman wrote: >> On 30-Apr-19 14:35, Dennis Dalessandro wrote: >>> On 4/30/2019 7:27 AM, Gal Pressman wrote: >>>> On 30-Apr-19 14:18, Leon Romanovsky wrote: >>>>> On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: >>>>>> Cited commit introduced the udata parameter to different destroy flows >>>>>> but the uapi method definition does not have udata (i.e has_udata flag >>>>>> is not set). As a result, an uninitialized udata struct is being passed >>>>>> down to the driver callbacks. >>>>>> >>>>>> Fix that by clearing the driver udata even in cases where has_udata flag >>>>>> is not set. >>>>>> >>>>>> Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") >>>>>> Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> >>>>>> Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> >>>>> >>>>> What is wrong with Signed-off-by that caused you to add new tag? >>>> >>>> Jason is the one that originally wrote and sent the code, this tag seems >>>> appropriate. >>>> Obviously I don't mind removing it, it's there to give him credit.. >>> >>> Did you find documentation for using that tag or did you just make it up? I >>> think Signed-off-by is what you want here. >> >> https://www.kernel.org/doc/html/v5.0/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by > > I see no benefit of this new tag over SOB, especially for the patch which > has 100% code of that Co-* author. I disagree, and probably more people as well as the tag was introduced to the kernel in addition to the SOB. Either way, I have no dog in this fight, the tag can be removed. Jason, let me know if I should resubmit without it.
On Tue, Apr 30, 2019 at 03:21:58PM +0300, Gal Pressman wrote: > On 30-Apr-19 15:07, Leon Romanovsky wrote: > > On Tue, Apr 30, 2019 at 02:38:30PM +0300, Gal Pressman wrote: > >> On 30-Apr-19 14:35, Dennis Dalessandro wrote: > >>> On 4/30/2019 7:27 AM, Gal Pressman wrote: > >>>> On 30-Apr-19 14:18, Leon Romanovsky wrote: > >>>>> On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: > >>>>>> Cited commit introduced the udata parameter to different destroy flows > >>>>>> but the uapi method definition does not have udata (i.e has_udata flag > >>>>>> is not set). As a result, an uninitialized udata struct is being passed > >>>>>> down to the driver callbacks. > >>>>>> > >>>>>> Fix that by clearing the driver udata even in cases where has_udata flag > >>>>>> is not set. > >>>>>> > >>>>>> Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") > >>>>>> Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > >>>>>> Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> > >>>>> > >>>>> What is wrong with Signed-off-by that caused you to add new tag? > >>>> > >>>> Jason is the one that originally wrote and sent the code, this tag seems > >>>> appropriate. > >>>> Obviously I don't mind removing it, it's there to give him credit.. > >>> > >>> Did you find documentation for using that tag or did you just make it up? I > >>> think Signed-off-by is what you want here. > >> > >> https://www.kernel.org/doc/html/v5.0/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by > > > > I see no benefit of this new tag over SOB, especially for the patch which > > has 100% code of that Co-* author. > > I disagree, and probably more people as well as the tag was introduced to the > kernel in addition to the SOB. This is exactly how bugs spread, one introduces it, other copy/paste it. > Either way, I have no dog in this fight, the tag can be removed. It doesn't matter, tried to figure why you added it and if i need to do it too. > > Jason, let me know if I should resubmit without it. There is no need to resubmit, Jason/Doug will remove it if they think differently from you.
On Tue, Apr 30, 2019 at 11:46:39AM +0300, Gal Pressman wrote: > Cited commit introduced the udata parameter to different destroy flows > but the uapi method definition does not have udata (i.e has_udata flag > is not set). As a result, an uninitialized udata struct is being passed > down to the driver callbacks. > > Fix that by clearing the driver udata even in cases where has_udata flag > is not set. > > Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") > Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca> > Signed-off-by: Gal Pressman <galpress@amazon.com> > --- > drivers/infiniband/core/uverbs_ioctl.c | 2 ++ > 1 file changed, 2 insertions(+) Applied to for-next thanks Jason
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c index cfbef25b3a73..829b0c6944d8 100644 --- a/drivers/infiniband/core/uverbs_ioctl.c +++ b/drivers/infiniband/core/uverbs_ioctl.c @@ -453,6 +453,8 @@ static int ib_uverbs_run_method(struct bundle_priv *pbundle, uverbs_fill_udata(&pbundle->bundle, &pbundle->bundle.driver_udata, UVERBS_ATTR_UHW_IN, UVERBS_ATTR_UHW_OUT); + else + pbundle->bundle.driver_udata = (struct ib_udata){}; if (destroy_bkey != UVERBS_API_ATTR_BKEY_LEN) { struct uverbs_obj_attr *destroy_attr =
Cited commit introduced the udata parameter to different destroy flows but the uapi method definition does not have udata (i.e has_udata flag is not set). As a result, an uninitialized udata struct is being passed down to the driver callbacks. Fix that by clearing the driver udata even in cases where has_udata flag is not set. Fixes: c4367a26357b ("IB: Pass uverbs_attr_bundle down ib_x destroy path") Cc: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> Co-developed-by: Jason Gunthorpe <jgg@ziepe.ca> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca> Signed-off-by: Gal Pressman <galpress@amazon.com> --- drivers/infiniband/core/uverbs_ioctl.c | 2 ++ 1 file changed, 2 insertions(+)