Messages in this thread Patch in this message | | | Date | Fri, 21 Apr 2006 14:42:27 +0200 | From | Tilman Schmidt <> | Subject | Re: [PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub |
| |
On 21.04.2006 08:14, Andrew Morton wrote: > It's simpler to code it this way: [snip] > But the code still looks wrong. If isdn_writebuf_stub() does a short write, we'll > just retry the entire write. And if that returns the same short write, we'll > retry the write again, ad infinitum.
You're right of course. But as i4l has never been able to handle that case correctly, i4l drivers just don't do short writes, period. They either return a negative error code, zero to indicate "not ready", or the requested length. Every i4l driver author learns that very quickly. (At least I did. ;-)
> One would expect that if a short write happened, we either bale out with an > error or we advance partway through the buffer and write some more.
isdn_write() is the write method of i4l's character device. If a short write would indeed occur, just passing it on to the caller should be ok, too. So I'd propose the following, even simpler version:
From: Jesper Juhl <jesper.juhl@gmail.com>
isdn_writebuf_stub() forgets to detect memory allocation and uaccess errors. And when that's fixed, if a error happens the caller will just keep on looping.
So change the caller to detect the error, and to return it.
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com> Cc: Karsten Keil <kkeil@suse.de> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
drivers/isdn/i4l/isdn_common.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff -puN linux-2.6.17-rc2-orig/drivers/isdn/i4l/isdn_common.c linux-2.6.17-rc2-work/drivers/isdn/i4l/isdn_common.c --- linux-2.6.17-rc2-orig/drivers/isdn/i4l/isdn_common.c 2006-03-20 06:53:29.000000000 +0100 +++ linux-2.6.17-rc2-work/drivers/isdn/i4l/isdn_common.c 2006-04-21 15:09:41.000000000 +0200 @@ -1177,9 +1177,8 @@ isdn_write(struct file *file, const char goto out; } chidx = isdn_minor2chan(minor); - while (isdn_writebuf_stub(drvidx, chidx, buf, count) != count) + while ((retval = isdn_writebuf_stub(drvidx, chidx, buf, count)) == 0) interruptible_sleep_on(&dev->drv[drvidx]->snd_waitq[chidx]); - retval = count; goto out; } if (minor <= ISDN_MINOR_CTRLMAX) { @@ -1951,9 +1950,10 @@ isdn_writebuf_stub(int drvidx, int chan, struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
if (!skb) - return 0; + return -ENOMEM; skb_reserve(skb, hl); - copy_from_user(skb_put(skb, len), buf, len); + if (!copy_from_user(skb_put(skb, len), buf, len)) + return -EFAULT; ret = dev->drv[drvidx]->interface->writebuf_skb(drvidx, chan, 1, skb); if (ret <= 0) dev_kfree_skb(skb); -- Tilman Schmidt E-Mail: tilman@imap.cc Bonn, Germany Reality is that which, when you stop believing in it, does not go away. (Philip K. Dick) [unhandled content-type:application/pgp-signature] | |