The refcount wrapper function vmci_ctx_get() may return NULL
context ptr. Thus, we need to add a NULL pointer check
before its dereference.
Signed-off-by: Xiyu Yang <[email protected]>
Signed-off-by: Xin Tan <[email protected]>
---
drivers/misc/vmw_vmci/vmci_context.c | 2 ++
drivers/misc/vmw_vmci/vmci_queue_pair.c | 17 +++++++++++------
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/misc/vmw_vmci/vmci_context.c b/drivers/misc/vmw_vmci/vmci_context.c
index 16695366ec92..a20878fba374 100644
--- a/drivers/misc/vmw_vmci/vmci_context.c
+++ b/drivers/misc/vmw_vmci/vmci_context.c
@@ -898,6 +898,8 @@ void vmci_ctx_rcv_notifications_release(u32 context_id,
bool success)
{
struct vmci_ctx *context = vmci_ctx_get(context_id);
+ if (context == NULL)
+ return;
spin_lock(&context->lock);
if (!success) {
diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c b/drivers/misc/vmw_vmci/vmci_queue_pair.c
index 8531ae781195..2ecb22d08692 100644
--- a/drivers/misc/vmw_vmci/vmci_queue_pair.c
+++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c
@@ -1575,11 +1575,14 @@ static int qp_broker_attach(struct qp_broker_entry *entry,
*/
create_context = vmci_ctx_get(entry->create_id);
- supports_host_qp = vmci_ctx_supports_host_qp(create_context);
- vmci_ctx_put(create_context);
+ if (!create_context) {
+ supports_host_qp =
+ vmci_ctx_supports_host_qp(create_context);
+ vmci_ctx_put(create_context);
- if (!supports_host_qp)
- return VMCI_ERROR_INVALID_RESOURCE;
+ if (!supports_host_qp)
+ return VMCI_ERROR_INVALID_RESOURCE;
+ }
}
if ((entry->qp.flags & ~VMCI_QP_ASYMM) != (flags & ~VMCI_QP_ASYMM_PEER))
@@ -1808,7 +1811,8 @@ static int qp_alloc_host_work(struct vmci_handle *handle,
pr_devel("queue pair broker failed to alloc (result=%d)\n",
result);
}
- vmci_ctx_put(context);
+ if (context)
+ vmci_ctx_put(context);
return result;
}
@@ -1859,7 +1863,8 @@ static int qp_detatch_host_work(struct vmci_handle handle)
result = vmci_qp_broker_detach(handle, context);
- vmci_ctx_put(context);
+ if (context)
+ vmci_ctx_put(context);
return result;
}
--
2.7.4
On Tue, Mar 17, 2020 at 02:29:57PM +0800, Xiyu Yang wrote:
> The refcount wrapper function vmci_ctx_get() may return NULL
> context ptr. Thus, we need to add a NULL pointer check
> before its dereference.
>
> Signed-off-by: Xiyu Yang <[email protected]>
> Signed-off-by: Xin Tan <[email protected]>
> ---
> drivers/misc/vmw_vmci/vmci_context.c | 2 ++
> drivers/misc/vmw_vmci/vmci_queue_pair.c | 17 +++++++++++------
> 2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/misc/vmw_vmci/vmci_context.c b/drivers/misc/vmw_vmci/vmci_context.c
> index 16695366ec92..a20878fba374 100644
> --- a/drivers/misc/vmw_vmci/vmci_context.c
> +++ b/drivers/misc/vmw_vmci/vmci_context.c
> @@ -898,6 +898,8 @@ void vmci_ctx_rcv_notifications_release(u32 context_id,
> bool success)
> {
> struct vmci_ctx *context = vmci_ctx_get(context_id);
> + if (context == NULL)
> + return;
Same comment as on your other patch.
And is this a v2?
>
> spin_lock(&context->lock);
> if (!success) {
> diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> index 8531ae781195..2ecb22d08692 100644
> --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c
> +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> @@ -1575,11 +1575,14 @@ static int qp_broker_attach(struct qp_broker_entry *entry,
> */
>
> create_context = vmci_ctx_get(entry->create_id);
> - supports_host_qp = vmci_ctx_supports_host_qp(create_context);
> - vmci_ctx_put(create_context);
> + if (!create_context) {
> + supports_host_qp =
> + vmci_ctx_supports_host_qp(create_context);
> + vmci_ctx_put(create_context);
>
> - if (!supports_host_qp)
> - return VMCI_ERROR_INVALID_RESOURCE;
> + if (!supports_host_qp)
> + return VMCI_ERROR_INVALID_RESOURCE;
> + }
> }
>
> if ((entry->qp.flags & ~VMCI_QP_ASYMM) != (flags & ~VMCI_QP_ASYMM_PEER))
> @@ -1808,7 +1811,8 @@ static int qp_alloc_host_work(struct vmci_handle *handle,
> pr_devel("queue pair broker failed to alloc (result=%d)\n",
> result);
> }
> - vmci_ctx_put(context);
> + if (context)
> + vmci_ctx_put(context);
can't vmci_ctx_put() take a NULL pointer? If not, fix this that way
please.
thanks,
greg k-h
Hi Greg,
On Wed, Mar 18, 2020 at 12:02:04PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 17, 2020 at 02:29:57PM +0800, Xiyu Yang wrote:
> > The refcount wrapper function vmci_ctx_get() may return NULL
> > context ptr. Thus, we need to add a NULL pointer check
> > before its dereference.
> >
> > Signed-off-by: Xiyu Yang <[email protected]>
> > Signed-off-by: Xin Tan <[email protected]>
> > ---
> > drivers/misc/vmw_vmci/vmci_context.c | 2 ++
> > drivers/misc/vmw_vmci/vmci_queue_pair.c | 17 +++++++++++------
> > 2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/misc/vmw_vmci/vmci_context.c b/drivers/misc/vmw_vmci/vmci_context.c
> > index 16695366ec92..a20878fba374 100644
> > --- a/drivers/misc/vmw_vmci/vmci_context.c
> > +++ b/drivers/misc/vmw_vmci/vmci_context.c
> > @@ -898,6 +898,8 @@ void vmci_ctx_rcv_notifications_release(u32 context_id,
> > bool success)
> > {
> > struct vmci_ctx *context = vmci_ctx_get(context_id);
> > + if (context == NULL)
> > + return;
>
> Same comment as on your other patch.
>
> And is this a v2?
Thanks! Yes, this is a v2.
According to our observation, vmci_ctx_rcv_notifications_release()
currently is only called by vmci_host_do_recv_notifications() which
guarantees a valid context object can be acquired with this context_id.
However, we argue that a NULL-check here is still necessary because
this function may be called by other functions in the future who may
fail/forget to provide such guarantee.
A NULL-check could gracely eliminiate such assumption on the callers
for vmci_ctx_rcv_notifications_release() with negligible overhead.
> > spin_lock(&context->lock);
> > if (!success) {
> > diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> > index 8531ae781195..2ecb22d08692 100644
> > --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c
> > +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> > @@ -1575,11 +1575,14 @@ static int qp_broker_attach(struct qp_broker_entry *entry,
> > */
> >
> > create_context = vmci_ctx_get(entry->create_id);
> > - supports_host_qp = vmci_ctx_supports_host_qp(create_context);
> > - vmci_ctx_put(create_context);
> > + if (!create_context) {
> > + supports_host_qp =
> > + vmci_ctx_supports_host_qp(create_context);
> > + vmci_ctx_put(create_context);
> >
> > - if (!supports_host_qp)
> > - return VMCI_ERROR_INVALID_RESOURCE;
> > + if (!supports_host_qp)
> > + return VMCI_ERROR_INVALID_RESOURCE;
> > + }
> > }
> >
> > if ((entry->qp.flags & ~VMCI_QP_ASYMM) != (flags & ~VMCI_QP_ASYMM_PEER))
> > @@ -1808,7 +1811,8 @@ static int qp_alloc_host_work(struct vmci_handle *handle,
> > pr_devel("queue pair broker failed to alloc (result=%d)\n",
> > result);
> > }
> > - vmci_ctx_put(context);
> > + if (context)
> > + vmci_ctx_put(context);
>
> can't vmci_ctx_put() take a NULL pointer? If not, fix this that way
> please.
>
> thanks,
>
> greg k-h
No. vmci_ctx_put() can not take a NULL pointer.
Sure, we will submit a new patch to perform a NULL-check in vmci_ctx_put().
On Mon, Mar 23, 2020 at 12:53:02PM +0800, Xiyu Yang wrote:
> Hi Greg,
>
> On Wed, Mar 18, 2020 at 12:02:04PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Mar 17, 2020 at 02:29:57PM +0800, Xiyu Yang wrote:
> > > The refcount wrapper function vmci_ctx_get() may return NULL
> > > context ptr. Thus, we need to add a NULL pointer check
> > > before its dereference.
> > >
> > > Signed-off-by: Xiyu Yang <[email protected]>
> > > Signed-off-by: Xin Tan <[email protected]>
> > > ---
> > > drivers/misc/vmw_vmci/vmci_context.c | 2 ++
> > > drivers/misc/vmw_vmci/vmci_queue_pair.c | 17 +++++++++++------
> > > 2 files changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/misc/vmw_vmci/vmci_context.c b/drivers/misc/vmw_vmci/vmci_context.c
> > > index 16695366ec92..a20878fba374 100644
> > > --- a/drivers/misc/vmw_vmci/vmci_context.c
> > > +++ b/drivers/misc/vmw_vmci/vmci_context.c
> > > @@ -898,6 +898,8 @@ void vmci_ctx_rcv_notifications_release(u32 context_id,
> > > bool success)
> > > {
> > > struct vmci_ctx *context = vmci_ctx_get(context_id);
> > > + if (context == NULL)
> > > + return;
> >
> > Same comment as on your other patch.
> >
> > And is this a v2?
>
> Thanks! Yes, this is a v2.
>
> According to our observation, vmci_ctx_rcv_notifications_release()
> currently is only called by vmci_host_do_recv_notifications() which
> guarantees a valid context object can be acquired with this context_id.
>
> However, we argue that a NULL-check here is still necessary because
> this function may be called by other functions in the future who may
> fail/forget to provide such guarantee.
No, that's not how we write code in the kernel, if it does not need to
be checked for because this can not happen, then do not check for it.
Don't try to plan for random users of your code in the future when you
control those users directly :)
thanks,
greg k-h
On Mon, Mar 23, 2020 at 7:55 AM Greg Kroah-Hartman
<[email protected]> wrote:
> On Mon, Mar 23, 2020 at 12:53:02PM +0800, Xiyu Yang wrote:
> > On Wed, Mar 18, 2020 at 12:02:04PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Mar 17, 2020 at 02:29:57PM +0800, Xiyu Yang wrote:
> > >
> > > Same comment as on your other patch.
> > >
> > > And is this a v2?
> >
> > Thanks! Yes, this is a v2.
> >
> > According to our observation, vmci_ctx_rcv_notifications_release()
> > currently is only called by vmci_host_do_recv_notifications() which
> > guarantees a valid context object can be acquired with this context_id.
> >
> > However, we argue that a NULL-check here is still necessary because
> > this function may be called by other functions in the future who may
> > fail/forget to provide such guarantee.
>
> No, that's not how we write code in the kernel, if it does not need to
> be checked for because this can not happen, then do not check for it.
>
> Don't try to plan for random users of your code in the future when you
> control those users directly :)
Just saw this reply after replying to the other mail. I guess I picked
a bad example ;-)
If there was in fact a report about a NULL pointer at put() time
somewhere, that would be the first thing to fix, and it may still
make sense to review the other code paths to see if there
are additional cases that can go wrong.
Arnd