Message ID | 1300307071-19665-6-git-send-email-jarod@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <mchehab@pedra> Envelope-to: mchehab@pedra Delivery-date: Wed, 16 Mar 2011 19:12:31 -0300 Received: from mchehab by pedra with local (Exim 4.72) (envelope-from <mchehab@pedra>) id 1PzyxC-0000YT-El for mchehab@pedra; Wed, 16 Mar 2011 19:12:30 -0300 Received: from casper.infradead.org [85.118.1.10] by pedra with IMAP (fetchmail-6.3.17) for <mchehab@localhost> (single-drop); Wed, 16 Mar 2011 19:12:30 -0300 (BRT) Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1PzxGx-0005fj-Us; Wed, 16 Mar 2011 20:24:48 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754040Ab1CPUYn (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Wed, 16 Mar 2011 16:24:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46332 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754035Ab1CPUYk (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 16 Mar 2011 16:24:40 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2GKOeaJ026214 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for <linux-media@vger.kernel.org>; Wed, 16 Mar 2011 16:24:40 -0400 Received: from xavier.bos.redhat.com (xavier.bos.redhat.com [10.16.16.50]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p2GKOXHk024620; Wed, 16 Mar 2011 16:24:39 -0400 From: Jarod Wilson <jarod@redhat.com> To: linux-media@vger.kernel.org Cc: Jarod Wilson <jarod@redhat.com> Subject: [PATCH 5/6] lirc_zilog: error out if buffer read bytes != chunk size Date: Wed, 16 Mar 2011 16:24:30 -0400 Message-Id: <1300307071-19665-6-git-send-email-jarod@redhat.com> In-Reply-To: <1300307071-19665-1-git-send-email-jarod@redhat.com> References: <1300307071-19665-1-git-send-email-jarod@redhat.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.11 Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org Sender: <mchehab@pedra> |
Commit Message
Jarod Wilson
March 16, 2011, 8:24 p.m. UTC
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
drivers/staging/lirc/lirc_zilog.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
Comments
On Wed, 2011-03-16 at 16:24 -0400, Jarod Wilson wrote: > Signed-off-by: Jarod Wilson <jarod@redhat.com> > --- > drivers/staging/lirc/lirc_zilog.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c > index 407d4b4..5ada643 100644 > --- a/drivers/staging/lirc/lirc_zilog.c > +++ b/drivers/staging/lirc/lirc_zilog.c > @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos) > ret = copy_to_user((void *)outbuf+written, buf, > rbuf->chunk_size); > written += rbuf->chunk_size; > + } else { > + zilog_error("Buffer read failed!\n"); > + ret = -EIO; > + break; No need to break, just let the non-0 ret value drop you out of the while loop. Regards, Andy > } > } > } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 16, 2011 at 08:07:22PM -0400, Andy Walls wrote: > On Wed, 2011-03-16 at 16:24 -0400, Jarod Wilson wrote: > > Signed-off-by: Jarod Wilson <jarod@redhat.com> > > --- > > drivers/staging/lirc/lirc_zilog.c | 4 ++++ > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c > > index 407d4b4..5ada643 100644 > > --- a/drivers/staging/lirc/lirc_zilog.c > > +++ b/drivers/staging/lirc/lirc_zilog.c > > @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos) > > ret = copy_to_user((void *)outbuf+written, buf, > > rbuf->chunk_size); > > written += rbuf->chunk_size; > > + } else { > > + zilog_error("Buffer read failed!\n"); > > + ret = -EIO; > > + break; > > No need to break, just let the non-0 ret value drop you out of the while > loop. Ah, indeed. I think I mindlessly copied what the tests just a few lines above were doing without looking at the actual reason for them. I'll remove that break from the patch here locally.
Jarod Wilson <jarod@redhat.com> wrote: >On Wed, Mar 16, 2011 at 08:07:22PM -0400, Andy Walls wrote: >> On Wed, 2011-03-16 at 16:24 -0400, Jarod Wilson wrote: >> > Signed-off-by: Jarod Wilson <jarod@redhat.com> >> > --- >> > drivers/staging/lirc/lirc_zilog.c | 4 ++++ >> > 1 files changed, 4 insertions(+), 0 deletions(-) >> > >> > diff --git a/drivers/staging/lirc/lirc_zilog.c >b/drivers/staging/lirc/lirc_zilog.c >> > index 407d4b4..5ada643 100644 >> > --- a/drivers/staging/lirc/lirc_zilog.c >> > +++ b/drivers/staging/lirc/lirc_zilog.c >> > @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, char >*outbuf, size_t n, loff_t *ppos) >> > ret = copy_to_user((void *)outbuf+written, buf, >> > rbuf->chunk_size); >> > written += rbuf->chunk_size; >> > + } else { >> > + zilog_error("Buffer read failed!\n"); >> > + ret = -EIO; >> > + break; >> >> No need to break, just let the non-0 ret value drop you out of the >while >> loop. > >Ah, indeed. I think I mindlessly copied what the tests just a few lines >above were doing without looking at the actual reason for them. I'll >remove that break from the patch here locally. > >-- >Jarod Wilson >jarod@redhat.com You might also want to take a look at that test to ensure it doesn't break blocking read() behavior. (man 2 read). I'm swamped ATM and didn't look too hard. It seems odd that the lirc buffer object can have data ready (the first branch of the big if() in the while() loop), and yet the read of that lirc buffer object fails. -Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 17, 2011 at 11:29:08AM -0400, Andy Walls wrote: > Jarod Wilson <jarod@redhat.com> wrote: > > >On Wed, Mar 16, 2011 at 08:07:22PM -0400, Andy Walls wrote: > >> On Wed, 2011-03-16 at 16:24 -0400, Jarod Wilson wrote: > >> > Signed-off-by: Jarod Wilson <jarod@redhat.com> > >> > --- > >> > drivers/staging/lirc/lirc_zilog.c | 4 ++++ > >> > 1 files changed, 4 insertions(+), 0 deletions(-) > >> > > >> > diff --git a/drivers/staging/lirc/lirc_zilog.c > >b/drivers/staging/lirc/lirc_zilog.c > >> > index 407d4b4..5ada643 100644 > >> > --- a/drivers/staging/lirc/lirc_zilog.c > >> > +++ b/drivers/staging/lirc/lirc_zilog.c > >> > @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, char > >*outbuf, size_t n, loff_t *ppos) > >> > ret = copy_to_user((void *)outbuf+written, buf, > >> > rbuf->chunk_size); > >> > written += rbuf->chunk_size; > >> > + } else { > >> > + zilog_error("Buffer read failed!\n"); > >> > + ret = -EIO; > >> > + break; > >> > >> No need to break, just let the non-0 ret value drop you out of the > >while > >> loop. > > > >Ah, indeed. I think I mindlessly copied what the tests just a few lines > >above were doing without looking at the actual reason for them. I'll > >remove that break from the patch here locally. > > > >-- > >Jarod Wilson > >jarod@redhat.com > > You might also want to take a look at that test to ensure it doesn't break blocking read() behavior. (man 2 read). I'm swamped ATM and didn't look too hard. > > It seems odd that the lirc buffer object can have data ready (the first branch of the big if() in the while() loop), and yet the read of that lirc buffer object fails. Generally, it shouldn't, but lirc_buffer_read uses kfifo underneath, and in the pre-2.6.33 kfifo implementation, the retval from lirc_buffer_read (as backported by way of media_build) is always 0, which is of course not equal to chunk_size. So I think that in current kernels, this should never trigger, and its partially just a note-to-self that this check will go sideways when running on an older kernel, but not a bad check to have if something really does go wrong.
Jarod Wilson <jarod@redhat.com> wrote: >On Thu, Mar 17, 2011 at 11:29:08AM -0400, Andy Walls wrote: >> Jarod Wilson <jarod@redhat.com> wrote: >> >> >On Wed, Mar 16, 2011 at 08:07:22PM -0400, Andy Walls wrote: >> >> On Wed, 2011-03-16 at 16:24 -0400, Jarod Wilson wrote: >> >> > Signed-off-by: Jarod Wilson <jarod@redhat.com> >> >> > --- >> >> > drivers/staging/lirc/lirc_zilog.c | 4 ++++ >> >> > 1 files changed, 4 insertions(+), 0 deletions(-) >> >> > >> >> > diff --git a/drivers/staging/lirc/lirc_zilog.c >> >b/drivers/staging/lirc/lirc_zilog.c >> >> > index 407d4b4..5ada643 100644 >> >> > --- a/drivers/staging/lirc/lirc_zilog.c >> >> > +++ b/drivers/staging/lirc/lirc_zilog.c >> >> > @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, >char >> >*outbuf, size_t n, loff_t *ppos) >> >> > ret = copy_to_user((void *)outbuf+written, buf, >> >> > rbuf->chunk_size); >> >> > written += rbuf->chunk_size; >> >> > + } else { >> >> > + zilog_error("Buffer read failed!\n"); >> >> > + ret = -EIO; >> >> > + break; >> >> >> >> No need to break, just let the non-0 ret value drop you out of the >> >while >> >> loop. >> > >> >Ah, indeed. I think I mindlessly copied what the tests just a few >lines >> >above were doing without looking at the actual reason for them. I'll >> >remove that break from the patch here locally. >> > >> >-- >> >Jarod Wilson >> >jarod@redhat.com >> >> You might also want to take a look at that test to ensure it doesn't >break blocking read() behavior. (man 2 read). I'm swamped ATM and >didn't look too hard. >> >> It seems odd that the lirc buffer object can have data ready (the >first branch of the big if() in the while() loop), and yet the read of >that lirc buffer object fails. > >Generally, it shouldn't, but lirc_buffer_read uses kfifo underneath, >and >in the pre-2.6.33 kfifo implementation, the retval from >lirc_buffer_read >(as backported by way of media_build) is always 0, which is of course >not >equal to chunk_size. So I think that in current kernels, this should >never >trigger, and its partially just a note-to-self that this check will go >sideways when running on an older kernel, but not a bad check to have >if >something really does go wrong. > >-- >Jarod Wilson >jarod@redhat.com But the orignal intent of the check I put in was to avoid passing partial/junk data to userspace, and go around again to see if good data could be provided. Your check bails when good data that might be sitting there still. That doesn't seem like a good trade for supporting backward compat for old kernels. -Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 17, 2011 at 12:16:31PM -0400, Andy Walls wrote: > Jarod Wilson <jarod@redhat.com> wrote: > > >On Thu, Mar 17, 2011 at 11:29:08AM -0400, Andy Walls wrote: > >> Jarod Wilson <jarod@redhat.com> wrote: > >> > >> >On Wed, Mar 16, 2011 at 08:07:22PM -0400, Andy Walls wrote: > >> >> On Wed, 2011-03-16 at 16:24 -0400, Jarod Wilson wrote: > >> >> > Signed-off-by: Jarod Wilson <jarod@redhat.com> > >> >> > --- > >> >> > drivers/staging/lirc/lirc_zilog.c | 4 ++++ > >> >> > 1 files changed, 4 insertions(+), 0 deletions(-) > >> >> > > >> >> > diff --git a/drivers/staging/lirc/lirc_zilog.c > >> >b/drivers/staging/lirc/lirc_zilog.c > >> >> > index 407d4b4..5ada643 100644 > >> >> > --- a/drivers/staging/lirc/lirc_zilog.c > >> >> > +++ b/drivers/staging/lirc/lirc_zilog.c > >> >> > @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, > >char > >> >*outbuf, size_t n, loff_t *ppos) > >> >> > ret = copy_to_user((void *)outbuf+written, buf, > >> >> > rbuf->chunk_size); > >> >> > written += rbuf->chunk_size; > >> >> > + } else { > >> >> > + zilog_error("Buffer read failed!\n"); > >> >> > + ret = -EIO; > >> >> > + break; > >> >> > >> >> No need to break, just let the non-0 ret value drop you out of the > >> >while > >> >> loop. > >> > > >> >Ah, indeed. I think I mindlessly copied what the tests just a few > >lines > >> >above were doing without looking at the actual reason for them. I'll > >> >remove that break from the patch here locally. > >> > > >> >-- > >> >Jarod Wilson > >> >jarod@redhat.com > >> > >> You might also want to take a look at that test to ensure it doesn't > >break blocking read() behavior. (man 2 read). I'm swamped ATM and > >didn't look too hard. > >> > >> It seems odd that the lirc buffer object can have data ready (the > >first branch of the big if() in the while() loop), and yet the read of > >that lirc buffer object fails. > > > >Generally, it shouldn't, but lirc_buffer_read uses kfifo underneath, > >and > >in the pre-2.6.33 kfifo implementation, the retval from > >lirc_buffer_read > >(as backported by way of media_build) is always 0, which is of course > >not > >equal to chunk_size. So I think that in current kernels, this should > >never > >trigger, and its partially just a note-to-self that this check will go > >sideways when running on an older kernel, but not a bad check to have > >if > >something really does go wrong. > > But the orignal intent of the check I put in was to avoid passing partial/junk data to userspace, and go around again to see if good data could be provided. > > Your check bails when good data that might be sitting there still. That doesn't seem like a good trade for supporting backward compat for old kernels. Ah. Another thing I neglected to notice then. :) Perhaps there should be a retry count check as well then, as otherwise, its possible to get stuck in that loop forever (which is what was happening on older kernels). Its conceivable that similar could happen on a newer kernel for some reason.
On Thu, 2011-03-17 at 15:08 -0400, Jarod Wilson wrote: > On Thu, Mar 17, 2011 at 12:16:31PM -0400, Andy Walls wrote: > > Jarod Wilson <jarod@redhat.com> wrote: > . > > > > But the orignal intent of the check I put in was to avoid passing > partial/junk data to userspace, and go around again to see if good > data could be provided. > > > > Your check bails when good data that might be sitting there still. > That doesn't seem like a good trade for supporting backward compat for > old kernels. > > Ah. Another thing I neglected to notice then. :) > > Perhaps there should be a retry count check as well then, as otherwise, > its possible to get stuck in that loop forever (which is what was > happening on older kernels). Its conceivable that similar could happen on > a newer kernel for some reason. Well, lets see, >From the perspective of userspace & lircd: 1. A specification compliance failure for a corner case isn't too bad (bailing out on junk and leaving good data behind) 2. An unrecoverable failure for any case is very bad (spinning/hanging on a result that won't change) 3. Sending unitialized bytes out to userspace with copy_to_user() is very bad. (I recall the old code would do the copy to user and always tell userspace it got a code whether it read anything out of the buffer or not. IIRC, that leaked information off the stack.) If the code as patched avoids the two very bad things (#2 and #3), then the patch is OK by me. Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 17, 2011 at 08:50:33PM -0400, Andy Walls wrote: > On Thu, 2011-03-17 at 15:08 -0400, Jarod Wilson wrote: > > On Thu, Mar 17, 2011 at 12:16:31PM -0400, Andy Walls wrote: > > > Jarod Wilson <jarod@redhat.com> wrote: > > . > > > > > > But the orignal intent of the check I put in was to avoid passing > > partial/junk data to userspace, and go around again to see if good > > data could be provided. > > > > > > Your check bails when good data that might be sitting there still. > > That doesn't seem like a good trade for supporting backward compat for > > old kernels. > > > > Ah. Another thing I neglected to notice then. :) > > > > Perhaps there should be a retry count check as well then, as otherwise, > > its possible to get stuck in that loop forever (which is what was > > happening on older kernels). Its conceivable that similar could happen on > > a newer kernel for some reason. > > Well, lets see, > > >From the perspective of userspace & lircd: > > 1. A specification compliance failure for a corner case isn't too bad > (bailing out on junk and leaving good data behind) > > 2. An unrecoverable failure for any case is very bad (spinning/hanging > on a result that won't change) > > 3. Sending unitialized bytes out to userspace with copy_to_user() is > very bad. > (I recall the old code would do the copy to user and always tell > userspace it got a code whether it read anything out of the buffer or > not. IIRC, that leaked information off the stack.) > > > If the code as patched avoids the two very bad things (#2 and #3), then > the patch is OK by me. I *think* what I've got now should address both 2 and 3, with a very minimal risk of leaving data behind, since it'll retry a couple of times before bailing out of the loop, so it should be pretty unlikely we'd leave any good data behind. But even if we do, like you said, this is just an IR signal, the user can press the button on the remote again. :)
diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c index 407d4b4..5ada643 100644 --- a/drivers/staging/lirc/lirc_zilog.c +++ b/drivers/staging/lirc/lirc_zilog.c @@ -950,6 +950,10 @@ static ssize_t read(struct file *filep, char *outbuf, size_t n, loff_t *ppos) ret = copy_to_user((void *)outbuf+written, buf, rbuf->chunk_size); written += rbuf->chunk_size; + } else { + zilog_error("Buffer read failed!\n"); + ret = -EIO; + break; } } }