Re: [PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub

[Tilman Schmidt <tilman@imap.cc>]

[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]

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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]