Skip to contentSkip to conversation

POSIX: Remove mutex/cond_init locking, fix NULL attribute

PATCH In review Jan Kiszka 3 participants 5 messages Apr 25, 2008

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 POSIX: Remove mutex/cond_init locking, fix NULL attribute

Hi,

trunk check-in #3369 did not just remove some "questionable critical sections", it also happen to fix two ugly bugs in the POSIX skin: The user was not able to pass NULL attributes down to mutex_init and cond_init. Find a backport of that patch for 2.4.x attached.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
ksrc/skins/posix/syscall.c +13 -51
Index: xenomai-2.4.x/ksrc/skins/posix/syscall.c
===================================================================
@@ -420,7 +420,6 @@ static int __sem_init(struct task_struct
 	union __xeno_sem sm, *usm;
 	unsigned value;
 	int pshared;
-	spl_t s;
 
 	usm = (union __xeno_sem *)__xn_reg_arg1(regs);
 

@@ -431,24 +430,18 @@ static int __sem_init(struct task_struct
 	pshared = (int)__xn_reg_arg2(regs);
 	value = (unsigned)__xn_reg_arg3(regs);
 
-	xnlock_get_irqsave(&nklock, s);
-	
 	__xn_copy_from_user(curr,
 			    &sm.shadow_sem,
 			    (void __user *)&usm->shadow_sem,
 			    sizeof(sm.shadow_sem));
 
-	if (sem_init(&sm.native_sem, pshared, value) == -1) {
-		xnlock_put_irqrestore(&nklock, s);
+	if (sem_init(&sm.native_sem, pshared, value) == -1)
 		return -thread_get_errno();
-	}
 
 	__xn_copy_to_user(curr,
 			  (void __user *)&usm->shadow_sem,
 			  &sm.shadow_sem, sizeof(usm->shadow_sem));
 
-	xnlock_put_irqrestore(&nklock, s);
-
 	return 0;
 }
 

@@ -1080,7 +1073,6 @@ static int __pthread_mutex_init(struct t
 {
 	pthread_mutexattr_t locattr, *attr, *uattrp;
 	union __xeno_mutex mx, *umx;
-	spl_t s;
 	int err;
 
 	umx = (union __xeno_mutex *)__xn_reg_arg1(regs);

@@ -1091,19 +1083,16 @@ static int __pthread_mutex_init(struct t
 	    (curr, VERIFY_WRITE, (void __user *)umx, sizeof(*umx)))
 		return -EFAULT;
 
-	if (!__xn_access_ok
-	    (curr, VERIFY_READ, (void __user *)uattrp, sizeof(*uattrp)))
-		return -EFAULT;
-
-	/* We want the initialization to be atomic. */
-	xnlock_get_irqsave(&nklock, s);
-
 	__xn_copy_from_user(curr,
 			    &mx.shadow_mutex,
 			    (void __user *)&umx->shadow_mutex,
 			    sizeof(mx.shadow_mutex));
 
 	if (uattrp) {
+		if (!__xn_access_ok
+		    (curr, VERIFY_READ, (void __user *)uattrp, sizeof(*uattrp)))
+			return -EFAULT;
+
 		__xn_copy_from_user(curr,
 				    &locattr,(void __user *)
 				    uattrp,

@@ -1115,17 +1104,13 @@ static int __pthread_mutex_init(struct t
 
 	err = pthread_mutex_init(&mx.native_mutex, attr);
 	
-	if (err) {
-		xnlock_put_irqrestore(&nklock, s);
+	if (err)
 		return -err;
-	}
 
 	__xn_copy_to_user(curr,
 			  (void __user *)&umx->shadow_mutex,
 			  &mx.shadow_mutex, sizeof(umx->shadow_mutex));
 
-	xnlock_put_irqrestore(&nklock, s);
-
 	return 0;
 }
 

@@ -1134,7 +1119,6 @@ static int __pthread_mutex_destroy(struc
 {
 	union __xeno_mutex mx, *umx;
 	int err;
-	spl_t s;
 
 	umx = (union __xeno_mutex *)__xn_reg_arg1(regs);
 

@@ -1142,8 +1126,6 @@ static int __pthread_mutex_destroy(struc
 	    (curr, VERIFY_READ, (void __user *)umx, sizeof(*umx)))
 		return -EFAULT;
 
-	xnlock_get_irqsave(&nklock, s);
-
 	__xn_copy_from_user(curr,
 			    &mx.shadow_mutex,
 			    (void __user *)&umx->shadow_mutex,

@@ -1151,17 +1133,13 @@ static int __pthread_mutex_destroy(struc
 
 	err = pthread_mutex_destroy(&mx.native_mutex);
 
-	if (err) {
-		xnlock_put_irqrestore(&nklock, s);
+	if (err)
 		return -err;
-	}
 	
 	__xn_copy_to_user(curr,
 			  (void __user *)&umx->shadow_mutex,
 			  &mx.shadow_mutex, sizeof(umx->shadow_mutex));
 
-	xnlock_put_irqrestore(&nklock, s);
-
 	return 0;
 }
 

@@ -1410,7 +1388,6 @@ static int __pthread_cond_init(struct ta
 {
 	pthread_condattr_t locattr, *attr, *uattrp;
 	union __xeno_cond cnd, *ucnd;
-	spl_t s;
 	int err;
 
 	ucnd = (union __xeno_cond *)__xn_reg_arg1(regs);

@@ -1421,19 +1398,16 @@ static int __pthread_cond_init(struct ta
 	    (curr, VERIFY_WRITE, (void __user *)ucnd, sizeof(*ucnd)))
 		return -EFAULT;
 
-	if (!__xn_access_ok
-	    (curr, VERIFY_READ, (void __user *)uattrp, sizeof(*uattrp)))
-		return -EFAULT;
-
-	/* We want the initialization to be atomic. */
-	xnlock_get_irqsave(&nklock, s);
-	
 	__xn_copy_from_user(curr,
 			    &cnd.shadow_cond,
 			    (void __user *)&ucnd->shadow_cond,
 			    sizeof(cnd.shadow_cond));
 
 	if (uattrp) {
+		if (!__xn_access_ok
+		    (curr, VERIFY_READ, (void __user *)uattrp, sizeof(*uattrp)))
+			return -EFAULT;
+
 		__xn_copy_from_user(curr,
 				    &locattr,
 				    (void __user *)uattrp,

@@ -1443,20 +1417,15 @@ static int __pthread_cond_init(struct ta
 	} else
 		attr = NULL;
 
-	/* Always use default attribute. */
 	err = pthread_cond_init(&cnd.native_cond, attr);
 
-	if (err) {
-		xnlock_put_irqrestore(&nklock, s);
+	if (err)
 		return -err;
-	}
 
 	__xn_copy_to_user(curr,
 			  (void __user *)&ucnd->shadow_cond,
 			  &cnd.shadow_cond, sizeof(ucnd->shadow_cond));
 
-	xnlock_put_irqrestore(&nklock, s);
-
 	return 0;
 }
 

@@ -1465,7 +1434,6 @@ static int __pthread_cond_destroy(struct
 {
 	union __xeno_cond cnd, *ucnd;
 	int err;
-	spl_t s;
 
 	ucnd = (union __xeno_cond *)__xn_reg_arg1(regs);
 

@@ -1473,8 +1441,6 @@ static int __pthread_cond_destroy(struct
 	    (curr, VERIFY_READ, (void __user *)ucnd, sizeof(*ucnd)))
 		return -EFAULT;
 
-	xnlock_get_irqsave(&nklock, s);
-
 	__xn_copy_from_user(curr,
 			    &cnd.shadow_cond,
 			    (void __user *)&ucnd->shadow_cond,

@@ -1482,17 +1448,13 @@ static int __pthread_cond_destroy(struct
 
 	err = pthread_cond_destroy(&cnd.native_cond);
 
-	if (err) {
-		xnlock_put_irqrestore(&nklock, s);
+	if (err)
 		return -err;
-	}
 
 	__xn_copy_to_user(curr,
 			  (void __user *)&ucnd->shadow_cond,
 			  &cnd.shadow_cond, sizeof(ucnd->shadow_cond));
 
-	xnlock_put_irqrestore(&nklock, s);
-
 	return 0;
 }
#1 lore raw
Quoted 6 lines · Jan Kiszka

Hi,

trunk check-in #3369 did not just remove some "questionable critical sections", it also happen to fix two ugly bugs in the POSIX skin: The user was not able to pass NULL attributes down to mutex_init and cond_init. Find a backport of that patch for 2.4.x attached.

I am using pthread_mutex_init and pthread_cond_init from user-space with NULL attributes with pre 2.4 versions all the time, and never had any problem.

Signature · 1 line
 Gilles
author
#2 lore raw
Quoted 11 lines · Gilles Chanteperdrix
Quoted 6 lines · Jan Kiszka

Hi,

trunk check-in #3369 did not just remove some "questionable critical sections", it also happen to fix two ugly bugs in the POSIX skin: The user was not able to pass NULL attributes down to mutex_init and cond_init. Find a backport of that patch for 2.4.x attached.

I am using pthread_mutex_init and pthread_cond_init from user-space with NULL attributes with pre 2.4 versions all the time, and never had any problem.

Maybe a x86-64 issue: __xn_access_ok() triggers here when you stuff in a NULL pointer.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
#3 lore raw
Quoted 10 lines · Jan Kiszka
Quoted 8 lines · Gilles Chanteperdrix
Quoted 6 lines · Jan Kiszka

Hi,

trunk check-in #3369 did not just remove some "questionable critical sections", it also happen to fix two ugly bugs in the POSIX skin: The user was not able to pass NULL attributes down to mutex_init and cond_init. Find a backport of that patch for 2.4.x attached.

Quoted 6 lines
Quoted 3 lines

I am using pthread_mutex_init and pthread_cond_init from user-space with NULL attributes with pre 2.4 versions all the time, and never had any problem.

Maybe a x86-64 issue: __xn_access_ok() triggers here when you stuff in a NULL pointer.

Ok. I had a look at the code now, and understood your patch. We simply have been lucky until now that xn_access_ok did not fail with NULL pointers.

Signature · 1 line
 Gilles
#4 lore raw
Quoted 14 lines · Gilles Chanteperdrix
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Gilles Chanteperdrix
Quoted 6 lines · Jan Kiszka

Hi,

trunk check-in #3369 did not just remove some "questionable critical sections", it also happen to fix two ugly bugs in the POSIX skin: The user was not able to pass NULL attributes down to mutex_init and cond_init. Find a backport of that patch for 2.4.x attached.

I am using pthread_mutex_init and pthread_cond_init from user-space with NULL attributes with pre 2.4 versions all the time, and never had any problem.

Quoted 7 lines
Quoted 2 lines

Maybe a x86-64 issue: __xn_access_ok() triggers here when you stuff in a NULL pointer.

Ok. I had a look at the code now, and understood your patch. We simply have been lucky until now that xn_access_ok did not fail with NULL pointers.

Unlike other ports, x86_64 specifically tests for page zero pointers...

#define __xn_access_ok(task,type,addr,size) ((unsigned long)(addr) >=

PAGE_SIZE && \
						__xn_range_ok(task, addr, size))
Signature · 1 line
Philippe.