[PATCH 0/3] media: atomisp: fix "don't pass a pointer to a local variable"

[PATCH 0/3] media: atomisp: fix "don't pass a pointer to a local variable"

[Hans de Goede]

Hi All,

While working on other atomisp stuff I noticed that the recently
added: "media: atomisp: don't pass a pointer to a local variable"
compiler warning fix broke things.

Here is a small series reverting the troublesome fix and adding
an alternative compiler warning fix which does work in my testing.

Regards,

Hans

p.s.

A while ago I mentioned that I was working on also making the code
work on Bay Trail devices (vs Cherry Trail) and that I had things
working with an older kernel based on Alan Cox' first merge of
the driver into drivers/staging. After a lot of work to keep the
code working rebasing on newer and newer (less old really) kernels
I had gathered some fixes and decided to just try the latest kernel.

And it turns out that the latest kernel already has all those
fixes and it just works. I don't know why my previous testing failed.
I might just have been unlucky with the hw which I used in my previous
testing.

So good news, the code works on Bay Trail too, which is also good
from a pov of being able to test on both platforms while doing further
refactoring.

Mauro, you also asked me to try mmap on the original code as merged
by Alan Cox, unfortunately mmap does not work their either, it seems
this has simply always been broken. More about this in another
patch-set.


Hans de Goede (3):
  media: atomisp: revert "don't pass a pointer to a local variable"
  media: atomisp: fix uninitialized stack mem usage in
    ia_css_rmgr_acq_vbuf()
  media: atomisp: fix -Wdangling-pointer warning

 .../atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c  | 22 +++++++++++++------
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
2.36.0

[PATCH 1/3] media: atomisp: revert "don't pass a pointer to a local variable"

[Hans de Goede]

The gcc is warning about returning a pointer to a local variable
is a false positive.

The type of handle is "struct ia_css_rmgr_vbuf_handle **" and
"h.vptr" is left to NULL, so the "if ((*handle)->vptr == 0x0)"
check always succeeds when the "*handle = &h;" statement which
gcc warns about executes. Leading to this statement being executed:

	rmgr_pop_handle(pool, handle);

If that succeeds,  then *handle has been set to point to one of
the pre-allocated array of handles, so it no longer points to h.

If that fails the following statement will be executed:

	/* Note that handle will change to an internally maintained one */
	ia_css_rmgr_refcount_retain_vbuf(handle);

Which allocated a new handle from the array of pre-allocated handles
and then makes *handle point to this. So the address of h is actually
never returned.

The fix for the false-postive compiler warning actually breaks the code,
the new:

	**handle = h;

is part of a "if (pool->copy_on_write) { ... }" which means that the
handle where *handle points to should be treated read-only, IOW
**handle must never be set, instead *handle must be set to point to
a new handle (with a copy of the contents of the old handle).

The old code correctly did this and the new fixed code gets this wrong.

Note there is another patch in this series, which fixes the warning
in another way.

Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c    | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c b/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
index 39604752785b..d96aaa4bc75d 100644
--- a/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
+++ b/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
@@ -254,7 +254,7 @@ void rmgr_pop_handle(struct ia_css_rmgr_vbuf_pool *pool,
 void ia_css_rmgr_acq_vbuf(struct ia_css_rmgr_vbuf_pool *pool,
 			  struct ia_css_rmgr_vbuf_handle **handle)
 {
-	struct ia_css_rmgr_vbuf_handle h = { 0 };
+	struct ia_css_rmgr_vbuf_handle h;
 
 	if ((!pool) || (!handle) || (!*handle)) {
 		IA_CSS_LOG("Invalid inputs");
@@ -272,7 +272,7 @@ void ia_css_rmgr_acq_vbuf(struct ia_css_rmgr_vbuf_pool *pool,
 			h.size = (*handle)->size;
 			/* release ref to current buffer */
 			ia_css_rmgr_refcount_release_vbuf(handle);
-			**handle = h;
+			*handle = &h;
 		}
 		/* get new buffer for needed size */
 		if ((*handle)->vptr == 0x0) {
-- 
2.36.0

[PATCH 2/3] media: atomisp: fix uninitialized stack mem usage in ia_css_rmgr_acq_vbuf()

[Hans de Goede]

When ia_css_rmgr_acq_vbuf() enters the code path where it uses the local
"struct ia_css_rmgr_vbuf_handle v" on the stack it relies on v.count==0
so that ia_css_rmgr_refcount_retain_vbuf allocates a new handle.

Explicitly set v.count to 0 rather then it being whatever was on the stack.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c b/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
index d96aaa4bc75d..afe2d22c603f 100644
--- a/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
+++ b/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
@@ -254,7 +254,7 @@ void rmgr_pop_handle(struct ia_css_rmgr_vbuf_pool *pool,
 void ia_css_rmgr_acq_vbuf(struct ia_css_rmgr_vbuf_pool *pool,
 			  struct ia_css_rmgr_vbuf_handle **handle)
 {
-	struct ia_css_rmgr_vbuf_handle h;
+	struct ia_css_rmgr_vbuf_handle h = { 0 };
 
 	if ((!pool) || (!handle) || (!*handle)) {
 		IA_CSS_LOG("Invalid inputs");
-- 
2.36.0

[PATCH 3/3] media: atomisp: fix -Wdangling-pointer warning

[Hans de Goede]

ia_css_rmgr_acq_vbuf() uses a local on stack
"struct ia_css_rmgr_vbuf_handle v" variable.

When this path using this is hit, either the rmgr_pop_handle() call
will make *handle point to another vbuf-handle, or because
v.count == 0, ia_css_rmgr_refcount_retain_vbuf() will alloc a new
vbuf-handle and make *handle point to it.

So on leaving the function *handle will never point to the on stack
vbuf-handle, but gcc does not know this and emits the following:

drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c: In function ‘ia_css_rmgr_acq_vbuf’:
drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c:276:33: warning: storing the address of local variable ‘h’ in ‘*handle’ [-Wdangling-pointer=]
  276 |                         *handle = &h;
      |                         ~~~~~~~~^~~~
drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c:257:40: note: ‘h’ declared here
  257 |         struct ia_css_rmgr_vbuf_handle h;
      |                                        ^
drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c:257:40: note: ‘handle’ declared here

Rework the code using a new_handle helper to suppress this
false-postive compiler warning.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c  | 22 +++++++++++++------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c b/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
index afe2d22c603f..b84c6cff1499 100644
--- a/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
+++ b/drivers/staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c
@@ -254,14 +254,15 @@ void rmgr_pop_handle(struct ia_css_rmgr_vbuf_pool *pool,
 void ia_css_rmgr_acq_vbuf(struct ia_css_rmgr_vbuf_pool *pool,
 			  struct ia_css_rmgr_vbuf_handle **handle)
 {
-	struct ia_css_rmgr_vbuf_handle h = { 0 };
-
 	if ((!pool) || (!handle) || (!*handle)) {
 		IA_CSS_LOG("Invalid inputs");
 		return;
 	}
 
 	if (pool->copy_on_write) {
+		struct ia_css_rmgr_vbuf_handle *new_handle;
+		struct ia_css_rmgr_vbuf_handle h = { 0 };
+
 		/* only one reference, reuse (no new retain) */
 		if ((*handle)->count == 1)
 			return;
@@ -272,23 +273,30 @@ void ia_css_rmgr_acq_vbuf(struct ia_css_rmgr_vbuf_pool *pool,
 			h.size = (*handle)->size;
 			/* release ref to current buffer */
 			ia_css_rmgr_refcount_release_vbuf(handle);
-			*handle = &h;
+			new_handle = &h;
+		} else {
+			new_handle = *handle;
 		}
 		/* get new buffer for needed size */
-		if ((*handle)->vptr == 0x0) {
+		if (new_handle->vptr == 0x0) {
 			if (pool->recycle) {
 				/* try and pop from pool */
-				rmgr_pop_handle(pool, handle);
+				rmgr_pop_handle(pool, &new_handle);
 			}
-			if ((*handle)->vptr == 0x0) {
+			if (new_handle->vptr == 0x0) {
 				/* we need to allocate */
-				(*handle)->vptr = hmm_alloc((*handle)->size,
+				new_handle->vptr = hmm_alloc(new_handle->size,
 							     HMM_BO_PRIVATE, 0, NULL, 0);
 			} else {
 				/* we popped a buffer */
+				*handle = new_handle;
 				return;
 			}
 		}
+		/* Note that new_handle will change to an internally maintained one */
+		ia_css_rmgr_refcount_retain_vbuf(&new_handle);
+		*handle = new_handle;
+		return;
 	}
 	/* Note that handle will change to an internally maintained one */
 	ia_css_rmgr_refcount_retain_vbuf(handle);
-- 
2.36.0

Re: [PATCH 1/3] media: atomisp: revert "don't pass a pointer to a local variable"

[Andy Shevchenko]

On Sun, Jun 12, 2022 at 6:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The gcc is warning about returning a pointer to a local variable
> is a false positive.
>
> The type of handle is "struct ia_css_rmgr_vbuf_handle **" and
> "h.vptr" is left to NULL, so the "if ((*handle)->vptr == 0x0)"
> check always succeeds when the "*handle = &h;" statement which
> gcc warns about executes. Leading to this statement being executed:
>
>         rmgr_pop_handle(pool, handle);
>
> If that succeeds,  then *handle has been set to point to one of
> the pre-allocated array of handles, so it no longer points to h.
>
> If that fails the following statement will be executed:
>
>         /* Note that handle will change to an internally maintained one */
>         ia_css_rmgr_refcount_retain_vbuf(handle);
>
> Which allocated a new handle from the array of pre-allocated handles
> and then makes *handle point to this. So the address of h is actually
> never returned.
>
> The fix for the false-postive compiler warning actually breaks the code,
> the new:
>
>         **handle = h;
>
> is part of a "if (pool->copy_on_write) { ... }" which means that the
> handle where *handle points to should be treated read-only, IOW
> **handle must never be set, instead *handle must be set to point to
> a new handle (with a copy of the contents of the old handle).
>
> The old code correctly did this and the new fixed code gets this wrong.
>
> Note there is another patch in this series, which fixes the warning
> in another way.

> Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")

Dunno for media subsystem, but for ones that Greg is maintain, the
point is that revert itself is already kinda fix and no need to have a
Fixes tag, instead the commit message should clearly have the
automatically generated line of revert (with the rest of the
explanation why that is needed). Just sharing my experience.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] media: atomisp: fix "don't pass a pointer to a local variable"

[Andy Shevchenko]

On Sun, Jun 12, 2022 at 6:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> While working on other atomisp stuff I noticed that the recently
> added: "media: atomisp: don't pass a pointer to a local variable"
> compiler warning fix broke things.
>
> Here is a small series reverting the troublesome fix and adding
> an alternative compiler warning fix which does work in my testing.
>
> Regards,
>
> Hans
>
> p.s.
>
> A while ago I mentioned that I was working on also making the code
> work on Bay Trail devices (vs Cherry Trail) and that I had things
> working with an older kernel based on Alan Cox' first merge of
> the driver into drivers/staging. After a lot of work to keep the
> code working rebasing on newer and newer (less old really) kernels
> I had gathered some fixes and decided to just try the latest kernel.
>
> And it turns out that the latest kernel already has all those
> fixes and it just works. I don't know why my previous testing failed.
> I might just have been unlucky with the hw which I used in my previous
> testing.
>
> So good news, the code works on Bay Trail too, which is also good
> from a pov of being able to test on both platforms while doing further
> refactoring.
>
> Mauro, you also asked me to try mmap on the original code as merged
> by Alan Cox, unfortunately mmap does not work their either, it seems
> this has simply always been broken. More about this in another
> patch-set.

Thanks for the fix!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Hans de Goede (3):
>   media: atomisp: revert "don't pass a pointer to a local variable"
>   media: atomisp: fix uninitialized stack mem usage in
>     ia_css_rmgr_acq_vbuf()
>   media: atomisp: fix -Wdangling-pointer warning
>
>  .../atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c  | 22 +++++++++++++------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> --
> 2.36.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] media: atomisp: revert "don't pass a pointer to a local variable"

[Dan Carpenter]

On Sun, Jun 12, 2022 at 09:22:55PM +0200, Andy Shevchenko wrote:
> > Note there is another patch in this series, which fixes the warning
> > in another way.
> 
> > Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")
> 
> Dunno for media subsystem, but for ones that Greg is maintain, the
> point is that revert itself is already kinda fix and no need to have a
> Fixes tag, instead the commit message should clearly have the
> automatically generated line of revert (with the rest of the
> explanation why that is needed). Just sharing my experience.

How would that work in this case?  We don't have a reference to the git
hash.

The `git revert` command came from early days of git and I always
feel like it hasn't keep up with how git is used these days.  The
subject doesn't have the subsystem prefix.  The commit message is wrong.
It uses the full git hash instead of the 12 char hash.  It doesn't have
a fixes tag.  Hans's commit is only correct because he re-wrote
basically everything.

Do a `git --grep=revert`.  Some of them you can grep for "This reverts
commit 8bdc2a190105e862dfe7a4033f2fd385b7e58ae8." but there are a lot
which are not machine parsable like:

bd06db5ff9af ("lib/flex_proportions.c: remove local_irq_ops in fprop_new_period()")
4af2bd190a5b ("Revert "squashfs: provide backing_dev_info in order to disable read-ahead"")
646728dff254 ("dmaengine: Revert "dmaengine: add verification of DMA_INTERRUPT capability for dmatest"")

I feel like we should encourage people to not use git revert because
otherwise we're kind of setting them up for failure.

regards,
dan carpenter

Re: [PATCH 1/3] media: atomisp: revert "don't pass a pointer to a local variable"

[Andy Shevchenko]

On Mon, Jun 13, 2022 at 05:58:20PM +0300, Dan Carpenter wrote:
> On Sun, Jun 12, 2022 at 09:22:55PM +0200, Andy Shevchenko wrote:
> > > Note there is another patch in this series, which fixes the warning
> > > in another way.
> > 
> > > Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")
> > 
> > Dunno for media subsystem, but for ones that Greg is maintain, the
> > point is that revert itself is already kinda fix and no need to have a
> > Fixes tag, instead the commit message should clearly have the
> > automatically generated line of revert (with the rest of the
> > explanation why that is needed). Just sharing my experience.
> 
> How would that work in this case?  We don't have a reference to the git
> hash.

What do you mean? `git revert` adds the hash of the commit being reverted.

> The `git revert` command came from early days of git and I always
> feel like it hasn't keep up with how git is used these days.  The
> subject doesn't have the subsystem prefix.  The commit message is wrong.
> It uses the full git hash instead of the 12 char hash.  It doesn't have
> a fixes tag.  Hans's commit is only correct because he re-wrote
> basically everything.
> 
> Do a `git --grep=revert`.  Some of them you can grep for "This reverts
> commit 8bdc2a190105e862dfe7a4033f2fd385b7e58ae8." but there are a lot
> which are not machine parsable

Why not? The format of the string hasn't been changed, no difference from other
patterns.

P.S. I have told this as my experience and what Greg told me, feel free to
discuss with him or others, I'm pretty much okay if Hans' patch goes as is.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] media: atomisp: revert "don't pass a pointer to a local variable"

[Dan Carpenter]

On Mon, Jun 13, 2022 at 06:39:19PM +0300, Andy Shevchenko wrote:
> > Do a `git --grep=revert`.  Some of them you can grep for "This reverts
> > commit 8bdc2a190105e862dfe7a4033f2fd385b7e58ae8." but there are a lot
> > which are not machine parsable
> 
> Why not? The format of the string hasn't been changed, no difference from other
> patterns.
> 

With the Fixes tag you can just do a:

	git log | grep Fixes: | cut -d : -f 2 | cut -d '(' -f 1

It's easily machine parseable.  But if you look at the examples I posted
they're stuff like this:

    This reverts commit 9eec1d897139e5d ("squashfs: provide backing_dev_info

It can't be grepped for, it needs a human to try figure it out.  And the
reason for that is that we always tell people that git hashes need to
be in a specific format which git revert violates.

Having two hashes *is* duplicative but if we're to delete a hash we
should do Hans did and delete the "This reverts commit
fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee." line.  (As an aside, in
that commit the reverts line is not a Fixes line.  The original commit
was a temporary hack and it was deleted when it was no longer required.
So reverts and Fixes are not the same.  Reverts is ambiguous.)

The problem with the reverts line is that most other people besides Greg
only look for the Fixes tags.  It had never occured to me to look for
the reverts line.  I was just reading an LWN article about bugs in
-stable and LWN only used Fixes tags, not reverts lines.  Or when people
are backporting patches I tell them to look for the Fixes tags to see if
they are backporting buggy patches.  If they're searching
lore.kernel.org most people will use the 12 char git hash instead of the
full hash.

My main problem with `git revert` is that it writes the commit message
for you and it does it really badly.  When I'm reviewing those patches
I have to tell people, "No, never use git revert format.  Send normal
patches."  I always tell them to redo it like Hans did.

Subject is wrong:
https://lore.kernel.org/all/20220614011528.32118-1-tangmeng@uniontech.com/

No Signed-off-by:
https://lore.kernel.org/all/BN9PR12MB5257FB6CA192626D5D108C2DFCAB9@BN9PR12MB5257.namprd12.prod.outlook.com/

Terrible commit message:
https://lore.kernel.org/all/20210414233533.24012-2-qingqing.zhuo@amd.com/

No commit message.
https://lore.kernel.org/all/20220613132116.2021055-2-idosch@nvidia.com/

These are just the first view I looked at from yesterday afternoon.
Almost every patch with Revert in the subject needs to be NAKed.

regards,
dan carpenter