Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Andy Walls 
On Fri, 2015-01-30 at 08:09 -0500, valdis.kletni...@vt.edu wrote:
> On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said:
> 
> > > > -   if (ir == NULL) {
> > > > -   dev_err(ir->l.dev, "close: no private_data attached to 
> > > > the file
> !\n");
> > >

commit be4aa8157c981a8bb9634b886bf1180f97205259
removed the dprintk(), which didn't depend on ir->l.dev, with this
dev_err() call.  That was the wrong thing to do. pr_info() is probably
the right thing to use, if one doesn't have a struct device instance.  

> > > Yes, the dev_err() call is an obvious thinko.
> > >
> > > However, I'm not sure whether removing it entirely is right either.  If
> > > there *should* be a struct IR * passed there, maybe some other printk()
> > > should be issued, or even a WARN_ON(!ir), or something?
> >
> > We set filep->private_data to non-NULL in open() so I don't think it can
> > be NULL here.
> 
> Then probably the *right* fix is to remove the *entire* if statement, as
> we can't end up doing the 'return -ENODEV'

The if() clause is here as an artifact of being part of a mass port of
lirc drivers from userspace.  I never removed it, because I needed it
when fixing all the lirc_zilog.c ref counting.

IF I got all the lirc_zilog ref counting right, and the upper layers of
the kernel never call close() in error, then this if() statement is not
needed.

I welcome anyone wishing to audit the ref-counting in lirc_zilog.  It
was mentally exhausting to get to what I think is right.  Maybe I just
tire easily mentally though. :)

-Andy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Rickard Strandqvist 
2015-01-30 14:09 GMT+01:00  :
> On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said:
>
>> > > - if (ir == NULL) {
>> > > - dev_err(ir->l.dev, "close: no private_data attached to the file
> !\n");
>> >
>> > Yes, the dev_err() call is an obvious thinko.
>> >
>> > However, I'm not sure whether removing it entirely is right either.  If
>> > there *should* be a struct IR * passed there, maybe some other printk()
>> > should be issued, or even a WARN_ON(!ir), or something?
>>
>> We set filep->private_data to non-NULL in open() so I don't think it can
>> be NULL here.
>
> Then probably the *right* fix is to remove the *entire* if statement, as
> we can't end up doing the 'return -ENODEV'


Hi

Ok, but think or know. Who knows?
Do the remove if patch?


Kind regards
Rickard Strandqvist
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Valdis . Kletnieks 
On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said:

> > > - if (ir == NULL) {
> > > - dev_err(ir->l.dev, "close: no private_data attached to the file
!\n");
> >
> > Yes, the dev_err() call is an obvious thinko.
> >
> > However, I'm not sure whether removing it entirely is right either.  If
> > there *should* be a struct IR * passed there, maybe some other printk()
> > should be issued, or even a WARN_ON(!ir), or something?
>
> We set filep->private_data to non-NULL in open() so I don't think it can
> be NULL here.

Then probably the *right* fix is to remove the *entire* if statement, as
we can't end up doing the 'return -ENODEV'


pgpZhdeUVOezO.pgp
Description: PGP signature

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Dan Carpenter 
On Thu, Jan 29, 2015 at 05:12:40PM -0500, valdis.kletni...@vt.edu wrote:
> On Thu, 29 Jan 2015 19:48:08 +0100, Rickard Strandqvist said:
> > Fix a possible null pointer dereference, there is
> > otherwise a risk of a possible null pointer dereference.
> >
> > This was found using a static code analysis program called cppcheck
> >
> > Signed-off-by: Rickard Strandqvist 
> > ---
> >  drivers/staging/media/lirc/lirc_zilog.c |4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> > /* find our IR struct */
> > struct IR *ir = filep->private_data;
> >
> > -   if (ir == NULL) {
> > -   dev_err(ir->l.dev, "close: no private_data attached to the 
> > file!\n");
> 
> Yes, the dev_err() call is an obvious thinko.
> 
> However, I'm not sure whether removing it entirely is right either.  If
> there *should* be a struct IR * passed there, maybe some other printk()
> should be issued, or even a WARN_ON(!ir), or something?

We set filep->private_data to non-NULL in open() so I don't think it can
be NULL here.

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Rickard Strandqvist 
2015-01-30 14:09 GMT+01:00  valdis.kletni...@vt.edu:
 On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said:

   - if (ir == NULL) {
   - dev_err(ir-l.dev, close: no private_data attached to the file
 !\n);
 
  Yes, the dev_err() call is an obvious thinko.
 
  However, I'm not sure whether removing it entirely is right either.  If
  there *should* be a struct IR * passed there, maybe some other printk()
  should be issued, or even a WARN_ON(!ir), or something?

 We set filep-private_data to non-NULL in open() so I don't think it can
 be NULL here.

 Then probably the *right* fix is to remove the *entire* if statement, as
 we can't end up doing the 'return -ENODEV'


Hi

Ok, but think or know. Who knows?
Do the remove if patch?


Kind regards
Rickard Strandqvist
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Andy Walls 
On Fri, 2015-01-30 at 08:09 -0500, valdis.kletni...@vt.edu wrote:
 On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said:
 
-   if (ir == NULL) {
-   dev_err(ir-l.dev, close: no private_data attached to 
the file
 !\n);
  

commit be4aa8157c981a8bb9634b886bf1180f97205259
removed the dprintk(), which didn't depend on ir-l.dev, with this
dev_err() call.  That was the wrong thing to do. pr_info() is probably
the right thing to use, if one doesn't have a struct device instance.  

   Yes, the dev_err() call is an obvious thinko.
  
   However, I'm not sure whether removing it entirely is right either.  If
   there *should* be a struct IR * passed there, maybe some other printk()
   should be issued, or even a WARN_ON(!ir), or something?
 
  We set filep-private_data to non-NULL in open() so I don't think it can
  be NULL here.
 
 Then probably the *right* fix is to remove the *entire* if statement, as
 we can't end up doing the 'return -ENODEV'

The if() clause is here as an artifact of being part of a mass port of
lirc drivers from userspace.  I never removed it, because I needed it
when fixing all the lirc_zilog.c ref counting.

IF I got all the lirc_zilog ref counting right, and the upper layers of
the kernel never call close() in error, then this if() statement is not
needed.

I welcome anyone wishing to audit the ref-counting in lirc_zilog.  It
was mentally exhausting to get to what I think is right.  Maybe I just
tire easily mentally though. :)

-Andy

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Dan Carpenter 
On Thu, Jan 29, 2015 at 05:12:40PM -0500, valdis.kletni...@vt.edu wrote:
 On Thu, 29 Jan 2015 19:48:08 +0100, Rickard Strandqvist said:
  Fix a possible null pointer dereference, there is
  otherwise a risk of a possible null pointer dereference.
 
  This was found using a static code analysis program called cppcheck
 
  Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
  ---
   drivers/staging/media/lirc/lirc_zilog.c |4 +---
   1 file changed, 1 insertion(+), 3 deletions(-)
 
  /* find our IR struct */
  struct IR *ir = filep-private_data;
 
  -   if (ir == NULL) {
  -   dev_err(ir-l.dev, close: no private_data attached to the 
  file!\n);
 
 Yes, the dev_err() call is an obvious thinko.
 
 However, I'm not sure whether removing it entirely is right either.  If
 there *should* be a struct IR * passed there, maybe some other printk()
 should be issued, or even a WARN_ON(!ir), or something?

We set filep-private_data to non-NULL in open() so I don't think it can
be NULL here.

regards,
dan carpenter


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-30 Thread Valdis . Kletnieks 
On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said:

   - if (ir == NULL) {
   - dev_err(ir-l.dev, close: no private_data attached to the file
!\n);
 
  Yes, the dev_err() call is an obvious thinko.
 
  However, I'm not sure whether removing it entirely is right either.  If
  there *should* be a struct IR * passed there, maybe some other printk()
  should be issued, or even a WARN_ON(!ir), or something?

 We set filep-private_data to non-NULL in open() so I don't think it can
 be NULL here.

Then probably the *right* fix is to remove the *entire* if statement, as
we can't end up doing the 'return -ENODEV'


pgpZhdeUVOezO.pgp
Description: PGP signature

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-29 Thread Valdis . Kletnieks 
On Thu, 29 Jan 2015 19:48:08 +0100, Rickard Strandqvist said:
> Fix a possible null pointer dereference, there is
> otherwise a risk of a possible null pointer dereference.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist 
> ---
>  drivers/staging/media/lirc/lirc_zilog.c |4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

>   /* find our IR struct */
>   struct IR *ir = filep->private_data;
>
> - if (ir == NULL) {
> - dev_err(ir->l.dev, "close: no private_data attached to the 
> file!\n");

Yes, the dev_err() call is an obvious thinko.

However, I'm not sure whether removing it entirely is right either.  If
there *should* be a struct IR * passed there, maybe some other printk()
should be issued, or even a WARN_ON(!ir), or something?


pgpqFHM7reilo.pgp
Description: PGP signature

[PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-29 Thread Rickard Strandqvist 
Fix a possible null pointer dereference, there is
otherwise a risk of a possible null pointer dereference.

This was found using a static code analysis program called cppcheck

Signed-off-by: Rickard Strandqvist 
---
 drivers/staging/media/lirc/lirc_zilog.c |4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index cc872fb..78ce3b0 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1332,10 +1332,8 @@ static int close(struct inode *node, struct file *filep)
/* find our IR struct */
struct IR *ir = filep->private_data;
 
-   if (ir == NULL) {
-   dev_err(ir->l.dev, "close: no private_data attached to the 
file!\n");
+   if (ir == NULL)
return -ENODEV;
-   }
 
atomic_dec(>open_count);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-29 Thread Rickard Strandqvist 
Fix a possible null pointer dereference, there is
otherwise a risk of a possible null pointer dereference.

This was found using a static code analysis program called cppcheck

Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
---
 drivers/staging/media/lirc/lirc_zilog.c |4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/media/lirc/lirc_zilog.c 
b/drivers/staging/media/lirc/lirc_zilog.c
index cc872fb..78ce3b0 100644
--- a/drivers/staging/media/lirc/lirc_zilog.c
+++ b/drivers/staging/media/lirc/lirc_zilog.c
@@ -1332,10 +1332,8 @@ static int close(struct inode *node, struct file *filep)
/* find our IR struct */
struct IR *ir = filep-private_data;
 
-   if (ir == NULL) {
-   dev_err(ir-l.dev, close: no private_data attached to the 
file!\n);
+   if (ir == NULL)
return -ENODEV;
-   }
 
atomic_dec(ir-open_count);
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: media: lirc: lirc_zilog: Fix for possible null pointer dereference

2015-01-29 Thread Valdis . Kletnieks 
On Thu, 29 Jan 2015 19:48:08 +0100, Rickard Strandqvist said:
 Fix a possible null pointer dereference, there is
 otherwise a risk of a possible null pointer dereference.

 This was found using a static code analysis program called cppcheck

 Signed-off-by: Rickard Strandqvist rickard_strandqv...@spectrumdigital.se
 ---
  drivers/staging/media/lirc/lirc_zilog.c |4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

   /* find our IR struct */
   struct IR *ir = filep-private_data;

 - if (ir == NULL) {
 - dev_err(ir-l.dev, close: no private_data attached to the 
 file!\n);

Yes, the dev_err() call is an obvious thinko.

However, I'm not sure whether removing it entirely is right either.  If
there *should* be a struct IR * passed there, maybe some other printk()
should be issued, or even a WARN_ON(!ir), or something?


pgpqFHM7reilo.pgp
Description: PGP signature