All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Two multipath-tools bug fixes
@ 2016-10-14 15:33 Bart Van Assche
  2016-10-14 15:34 ` [PATCH 1/2] multipathd: Avoid "socket operation on non-socket" errors Bart Van Assche
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Bart Van Assche @ 2016-10-14 15:33 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development

Hello Christophe,

Please consider the two patches in this series for inclusion in the 
multipath-tools repository. The first patch fixes a recently introduced 
bug and the second patch fixes a longstanding bug.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] multipathd: Avoid "socket operation on non-socket" errors
  2016-10-14 15:33 [PATCH 0/2] Two multipath-tools bug fixes Bart Van Assche
@ 2016-10-14 15:34 ` Bart Van Assche
  2016-10-14 15:35 ` [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown Bart Van Assche
  2016-10-16  7:52 ` [PATCH 0/2] Two multipath-tools bug fixes Christophe Varoqui
  2 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2016-10-14 15:34 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development

Changing write_all() into write() is safe for files since the
POSIX standard guarantees that write() writes the entire buffer
except if the disk is full, a resource limit is encountered or
if interrupted by a signal. See also
http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html.

Fixes: commit 810082e7a8cf ("libmultipath, multipathd: Rework SIGPIPE handling")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/alias.c | 2 +-
 libmultipath/file.c  | 2 +-
 libmultipath/wwids.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index b86843a..12afef8 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -219,7 +219,7 @@ allocate_binding(int fd, char *wwid, int id, char *prefix)
 			strerror(errno));
 		return NULL;
 	}
-	if (write_all(fd, buf, strlen(buf)) != strlen(buf)){
+	if (write(fd, buf, strlen(buf)) != strlen(buf)){
 		condlog(0, "Cannot write binding to bindings file : %s",
 			strerror(errno));
 		/* clear partial write */
diff --git a/libmultipath/file.c b/libmultipath/file.c
index 74cde64..e4951c9 100644
--- a/libmultipath/file.c
+++ b/libmultipath/file.c
@@ -158,7 +158,7 @@ open_file(char *file, int *can_write, char *header)
 			goto fail;
 		/* If file is empty, write the header */
 		size_t len = strlen(header);
-		if (write_all(fd, header, len) != len) {
+		if (write(fd, header, len) != len) {
 			condlog(0,
 				"Cannot write header to file %s : %s", file,
 				strerror(errno));
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index babf149..bc70a27 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -71,7 +71,7 @@ write_out_wwid(int fd, char *wwid) {
 			strerror(errno));
 		return -1;
 	}
-	if (write_all(fd, buf, strlen(buf)) != strlen(buf)) {
+	if (write(fd, buf, strlen(buf)) != strlen(buf)) {
 		condlog(0, "cannot write wwid to wwids file : %s",
 			strerror(errno));
 		if (ftruncate(fd, offset))
@@ -110,7 +110,7 @@ replace_wwids(vector mp)
 		goto out_file;
 	}
 	len = strlen(WWIDS_FILE_HEADER);
-	if (write_all(fd, WWIDS_FILE_HEADER, len) != len) {
+	if (write(fd, WWIDS_FILE_HEADER, len) != len) {
 		condlog(0, "Can't write wwid file header : %s",
 			strerror(errno));
 		/* cleanup partially written header */
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown
  2016-10-14 15:33 [PATCH 0/2] Two multipath-tools bug fixes Bart Van Assche
  2016-10-14 15:34 ` [PATCH 1/2] multipathd: Avoid "socket operation on non-socket" errors Bart Van Assche
@ 2016-10-14 15:35 ` Bart Van Assche
  2016-10-20 22:28   ` Xose Vazquez Perez
  2016-10-16  7:52 ` [PATCH 0/2] Two multipath-tools bug fixes Christophe Varoqui
  2 siblings, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2016-10-14 15:35 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development

pthread_cond_wait() is a thread cancellation point. If a thread that
is waiting in pthread_cond_wait() is canceled it is possible that the
mutex is re-acquired before the first cancellation cleanup handler
is called. In this case the cleanup handler is uevq_stop() and that
function locks uevq_lock. Avoid that calling uevq_stop() results in
a deadlock due to an attempt to lock a non-recursive mutex recursively.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/uevent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 6247898..85fd2fb 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -52,7 +52,7 @@ typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
 pthread_t uevq_thr;
 LIST_HEAD(uevq);
-pthread_mutex_t uevq_lock = PTHREAD_MUTEX_INITIALIZER;
+pthread_mutex_t uevq_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
 pthread_mutex_t *uevq_lockp = &uevq_lock;
 pthread_cond_t uev_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t *uev_condp = &uev_cond;
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Two multipath-tools bug fixes
  2016-10-14 15:33 [PATCH 0/2] Two multipath-tools bug fixes Bart Van Assche
  2016-10-14 15:34 ` [PATCH 1/2] multipathd: Avoid "socket operation on non-socket" errors Bart Van Assche
  2016-10-14 15:35 ` [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown Bart Van Assche
@ 2016-10-16  7:52 ` Christophe Varoqui
  2 siblings, 0 replies; 6+ messages in thread
From: Christophe Varoqui @ 2016-10-16  7:52 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: device-mapper development


[-- Attachment #1.1: Type: text/plain, Size: 347 bytes --]

Merged.
Thanks.

On Fri, Oct 14, 2016 at 5:33 PM, Bart Van Assche <bart.vanassche@sandisk.com
> wrote:

> Hello Christophe,
>
> Please consider the two patches in this series for inclusion in the
> multipath-tools repository. The first patch fixes a recently introduced bug
> and the second patch fixes a longstanding bug.
>
> Thanks,
>
> Bart.
>

[-- Attachment #1.2: Type: text/html, Size: 675 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown
  2016-10-14 15:35 ` [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown Bart Van Assche
@ 2016-10-20 22:28   ` Xose Vazquez Perez
  2016-10-20 22:47     ` Bart Van Assche
  0 siblings, 1 reply; 6+ messages in thread
From: Xose Vazquez Perez @ 2016-10-20 22:28 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: device-mapper development

On 10/14/2016 05:35 PM, Bart Van Assche wrote:

> pthread_cond_wait() is a thread cancellation point. If a thread that
> is waiting in pthread_cond_wait() is canceled it is possible that the
> mutex is re-acquired before the first cancellation cleanup handler
> is called. In this case the cleanup handler is uevq_stop() and that
> function locks uevq_lock. Avoid that calling uevq_stop() results in
> a deadlock due to an attempt to lock a non-recursive mutex recursively.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
>  libmultipath/uevent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 6247898..85fd2fb 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -52,7 +52,7 @@ typedef int (uev_trigger)(struct uevent *, void * trigger_data);
>  
>  pthread_t uevq_thr;
>  LIST_HEAD(uevq);
> -pthread_mutex_t uevq_lock = PTHREAD_MUTEX_INITIALIZER;
> +pthread_mutex_t uevq_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
>  pthread_mutex_t *uevq_lockp = &uevq_lock;
>  pthread_cond_t uev_cond = PTHREAD_COND_INITIALIZER;
>  pthread_cond_t *uev_condp = &uev_cond;
> 

PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP is not defined in POSIX.

This is going to be problematic in musl-libc based distributions:
http://wiki.musl-libc.org/wiki/Projects_using_musl#Distributions


~/musl-gcc -O2 -g -pipe -Wall -Wextra -Wformat=2 -Wno-sign-compare -Wno-unused-parameter -Wno-clobbered -Wp,-D_FORTIFY_SOURCE=2 -fstack-protector --param=ssp-buffer-size=4 -fPIC -DLIB_STRING=\"lib64\"
-DRUN_DIR=\"run\" -I../libmpathcmd -DUSE_SYSTEMD=229 -DLIBDM_API_FLUSH -D_GNU_SOURCE -DLIBDM_API_COOKIE -DLIBUDEV_API_RECVBUF -DLIBDM_API_DEFERRED -c -o uevent.o uevent.c
uevent.c:55:29: error: ‘PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP’ undeclared here (not in a function)
 pthread_mutex_t uevq_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../Makefile.inc:74: recipe for target 'uevent.o' failed
make[1]: [uevent.o] Error 1 (ignored)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown
  2016-10-20 22:28   ` Xose Vazquez Perez
@ 2016-10-20 22:47     ` Bart Van Assche
  0 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2016-10-20 22:47 UTC (permalink / raw)
  To: Xose Vazquez Perez; +Cc: device-mapper development

On 10/20/2016 03:28 PM, Xose Vazquez Perez wrote:
> On 10/14/2016 05:35 PM, Bart Van Assche wrote:
>
>> pthread_cond_wait() is a thread cancellation point. If a thread that
>> is waiting in pthread_cond_wait() is canceled it is possible that the
>> mutex is re-acquired before the first cancellation cleanup handler
>> is called. In this case the cleanup handler is uevq_stop() and that
>> function locks uevq_lock. Avoid that calling uevq_stop() results in
>> a deadlock due to an attempt to lock a non-recursive mutex recursively.
>>
>> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
>> ---
>>  libmultipath/uevent.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
>> index 6247898..85fd2fb 100644
>> --- a/libmultipath/uevent.c
>> +++ b/libmultipath/uevent.c
>> @@ -52,7 +52,7 @@ typedef int (uev_trigger)(struct uevent *, void * trigger_data);
>>
>>  pthread_t uevq_thr;
>>  LIST_HEAD(uevq);
>> -pthread_mutex_t uevq_lock = PTHREAD_MUTEX_INITIALIZER;
>> +pthread_mutex_t uevq_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
>>  pthread_mutex_t *uevq_lockp = &uevq_lock;
>>  pthread_cond_t uev_cond = PTHREAD_COND_INITIALIZER;
>>  pthread_cond_t *uev_condp = &uev_cond;
>>
>
> PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP is not defined in POSIX.
>
> This is going to be problematic in musl-libc based distributions:
> http://wiki.musl-libc.org/wiki/Projects_using_musl#Distributions
>
>
> ~/musl-gcc -O2 -g -pipe -Wall -Wextra -Wformat=2 -Wno-sign-compare -Wno-unused-parameter -Wno-clobbered -Wp,-D_FORTIFY_SOURCE=2 -fstack-protector --param=ssp-buffer-size=4 -fPIC -DLIB_STRING=\"lib64\"
> -DRUN_DIR=\"run\" -I../libmpathcmd -DUSE_SYSTEMD=229 -DLIBDM_API_FLUSH -D_GNU_SOURCE -DLIBDM_API_COOKIE -DLIBUDEV_API_RECVBUF -DLIBDM_API_DEFERRED -c -o uevent.o uevent.c
> uevent.c:55:29: error: ‘PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP’ undeclared here (not in a function)
>  pthread_mutex_t uevq_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
>                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../Makefile.inc:74: recipe for target 'uevent.o' failed
> make[1]: [uevent.o] Error 1 (ignored)

Hello Xose,

Thanks for reporting this. I will rework that patch.

Bart.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-20 22:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-14 15:33 [PATCH 0/2] Two multipath-tools bug fixes Bart Van Assche
2016-10-14 15:34 ` [PATCH 1/2] multipathd: Avoid "socket operation on non-socket" errors Bart Van Assche
2016-10-14 15:35 ` [PATCH 2/2] multipathd: Avoid that a deadlock is triggered sporadically during shutdown Bart Van Assche
2016-10-20 22:28   ` Xose Vazquez Perez
2016-10-20 22:47     ` Bart Van Assche
2016-10-16  7:52 ` [PATCH 0/2] Two multipath-tools bug fixes Christophe Varoqui

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.