2006-11-26 10:01:10

by Eugene Teo

Subject: [2.6 patch] net/sctp/socket.c: add missing sctp_spin_unlock_irqrestore

This patch adds a missing sctp_spin_unlock_irqrestore when returning
from "if(space_left<addrlen)" condition.

Signed-off-by: Eugene Teo <[email protected]>

net/sctp/socket.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 935bc91..a5d4d75 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3955,7 +3955,7 @@ static int sctp_copy_laddrs_to_user(stru
struct sctp_sockaddr_entry *addr;
unsigned long flags;
union sctp_addr temp;
- int cnt = 0;
+ int cnt = 0, err = 0;
int addrlen;

sctp_spin_lock_irqsave(&sctp_local_addr_lock, flags);
@@ -3968,21 +3968,24 @@ static int sctp_copy_laddrs_to_user(stru
sctp_get_pf_specific(sk->sk_family)->addr_v4map(sctp_sk(sk),
&temp);
addrlen = sctp_get_af_specific(temp.sa.sa_family)->sockaddr_len;
- if(space_left<addrlen)
- return -ENOMEM;
+ if(space_left<addrlen) {
+ err = -ENOMEM;
+ goto unlock;
+ }
temp.v4.sin_port = htons(port);
if (copy_to_user(*to, &temp, addrlen)) {
- sctp_spin_unlock_irqrestore(&sctp_local_addr_lock,
- flags);
- return -EFAULT;
+ err = -EFAULT;
+ goto unlock;
}
*to += addrlen;
cnt ++;
space_left -= addrlen;
}
- sctp_spin_unlock_irqrestore(&sctp_local_addr_lock, flags);
+ err = cnt;

- return cnt;
+unlock:
+ sctp_spin_unlock_irqrestore(&sctp_local_addr_lock, flags);
+ return err;
}

/* Old API for getting list of local addresses. Does not work for 32-bit


--
eteo redhat.com ph: +65 6490 4142 http://www.kernel.org/~eugeneteo
gpg fingerprint: 47B9 90F6 AE4A 9C51 37E0 D6E1 EA84 C6A2 58DF 8823

2006-11-26 10:12:58

by Al Viro

Subject: Re: [2.6 patch] net/sctp/socket.c: add missing sctp_spin_unlock_irqrestore

On Sun, Nov 26, 2006 at 06:00:53PM +0800, Eugene Teo wrote:
> This patch adds a missing sctp_spin_unlock_irqrestore when returning
> from "if(space_left<addrlen)" condition.
> if (copy_to_user(*to, &temp, addrlen)) {
> - sctp_spin_unlock_irqrestore(&sctp_local_addr_lock,
> - flags);
> - return -EFAULT;
> + err = -EFAULT;
> + goto unlock;

> + sctp_spin_unlock_irqrestore(&sctp_local_addr_lock, flags);
> + return err;
> }

You do realize that it's obviously still badly broken, don't you?
copy_to_user() under a spinlock is a recipe for deadlock, especially
if you've got interrupts disabled...

I have a beginning of locking fixes in that shitpile, but it's incomplete
and bloody painful ;-/

2006-11-26 10:19:23

by Arjan van de Ven

Subject: Re: [2.6 patch] net/sctp/socket.c: add missing sctp_spin_unlock_irqrestore

On Sun, 2006-11-26 at 10:12 +0000, Al Viro wrote:
> On Sun, Nov 26, 2006 at 06:00:53PM +0800, Eugene Teo wrote:
> > This patch adds a missing sctp_spin_unlock_irqrestore when returning
> > from "if(space_left<addrlen)" condition.
> > if (copy_to_user(*to, &temp, addrlen)) {
> > - sctp_spin_unlock_irqrestore(&sctp_local_addr_lock,
> > - flags);
> > - return -EFAULT;
> > + err = -EFAULT;
> > + goto unlock;
>
> > + sctp_spin_unlock_irqrestore(&sctp_local_addr_lock, flags);
> > + return err;
> > }
>
> You do realize that it's obviously still badly broken, don't you?
> copy_to_user() under a spinlock is a recipe for deadlock, especially
> if you've got interrupts disabled...
>
> I have a beginning of locking fixes in that shitpile, but it's incomplete
> and bloody painful ;-/
> -

do your patches also remove the empty abstraction of the sctp_ prefix
around the spinlock use in sctp ?


--
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

2006-11-26 12:38:12

by Eugene Teo

Subject: Re: [2.6 patch] net/sctp/socket.c: add missing sctp_spin_unlock_irqrestore

Al Viro wrote:
> On Sun, Nov 26, 2006 at 06:00:53PM +0800, Eugene Teo wrote:
>> This patch adds a missing sctp_spin_unlock_irqrestore when returning
>> from "if(space_left<addrlen)" condition.
>> if (copy_to_user(*to, &temp, addrlen)) {
>> - sctp_spin_unlock_irqrestore(&sctp_local_addr_lock,
>> - flags);
>> - return -EFAULT;
>> + err = -EFAULT;
>> + goto unlock;
>
>> + sctp_spin_unlock_irqrestore(&sctp_local_addr_lock, flags);
>> + return err;
>> }
>
> You do realize that it's obviously still badly broken, don't you?
> copy_to_user() under a spinlock is a recipe for deadlock, especially
> if you've got interrupts disabled...

Realized. Back to drawing board.

Eugene
--
1024D/58DF8823 print 47B9 90F6 AE4A 9C51 37E0 D6E1 EA84 C6A2 58DF 8823
main(i) { putchar(182623909 >> (i-1) * 5&31|!!(i<7)<<6) && main(++i); }