Re: [dm-devel] [PATCH V7 2/4] multipath-tools: New way to limit the IPC command length.

2016-08-12 Thread Benjamin Marzinski 
On Fri, Aug 12, 2016 at 08:12:38PM +0800, Gris Ge wrote:

Perhaps a better way would be to keep your v6 code, and just make the
client ignore the SIGPIPE signals. Personally, I'm fine with always
ignoring them, since the writes will still fail with EPIPE, and
the multipathd client code should be dealing with failed writes
correctly.  If people are worried that ignoring SIGPIPE will cause
unchecked failures to stdout/stderr. Then I'm fine with just blocking
SIGPIPE during socket writes, so that we can print out a useful error
message before we die.

-Ben

> Problem:
> 
> mpath_recv_reply() return -EINVAL on command 'show maps json' with 2k 
> paths.
> 
> Root cause:
> 
> Commit 174e717d351789a3cb29e1417f8e910baabcdb16 introduced the
> limitation on max bytes(65535) of reply string from multipathd.
> With 2k paths(1k mpaths) simulated by scsi_debug, the 'show maps json'
> requires 1633217 bytes which trigged the EINVAL error.
> 
> Fix:
> * Remove the limitation of MAX_REPLY_LEN in libmpathcmd.
> 
> * Remove uxsock.h from libmultipath, changed all non-daemon usage to
>   libmpathcmd instead.
> 
> * Rename send_packet() to send_packet_daemon_only() which is
>   dedicated for multipathd socket listener.
> 
> * Rename recv_packet() to recv_packet_daemon_only() which is
>   dedicate for multipathd socket listener.
> 
> * Enforce limitation(255) of IPC command string in
>   recv_packet_daemon_only().
> 
> * Removed unused read_all() from uxsock.h.
> 
> * Moved write_all() to file.h of libmultipath as all usage of
>   write_all() is against file rather than socket.
> 
> Changes caused by patch:
> 
> * Data sent from IPC client(including multipathd -k) to multipathd
>   is limited to 255 bytes maximum. This prevent malicious IPC client
>   send something like 'fake command' to daemon which
>   lead daemon to allocate a big chunk of memory.
> 
> * Due to the removal of uxsock.h from libmultipath, all IPC connection
>   except (multipathd daemon) should use libmpathcmd instead.
> 
> Signed-off-by: Gris Ge 
> ---
>  libmpathcmd/mpath_cmd.c   |  2 -
>  libmpathcmd/mpath_cmd.h   |  2 -
>  libmpathpersist/mpath_updatepr.c  |  8 +---
>  libmultipath/Makefile |  2 +-
>  libmultipath/alias.c  |  1 -
>  libmultipath/configure.c  |  5 +-
>  libmultipath/file.c   | 24 +-
>  libmultipath/file.h   |  1 +
>  libmultipath/uxsock.h |  6 ---
>  libmultipath/wwids.c  |  1 -
>  multipath/main.c  |  1 -
>  multipathd/Makefile   |  2 +-
>  multipathd/uxclnt.c   | 16 +++
>  multipathd/uxlsnr.c   | 10 ++--
>  {libmultipath => multipathd}/uxsock.c | 89 
> +++
>  multipathd/uxsock.h   |  8 
>  16 files changed, 55 insertions(+), 123 deletions(-)
>  delete mode 100644 libmultipath/uxsock.h
>  rename {libmultipath => multipathd}/uxsock.c (57%)
>  create mode 100644 multipathd/uxsock.h
> 
> diff --git a/libmpathcmd/mpath_cmd.c b/libmpathcmd/mpath_cmd.c
> index 0daaf53..b400038 100644
> --- a/libmpathcmd/mpath_cmd.c
> +++ b/libmpathcmd/mpath_cmd.c
> @@ -156,8 +156,6 @@ int mpath_recv_reply(int fd, char **reply, unsigned int 
> timeout)
>   len = mpath_recv_reply_len(fd, timeout);
>   if (len <= 0)
>   return len;
> - if (len > MAX_REPLY_LEN)
> - return -EINVAL;
>   *reply = malloc(len);
>   if (!*reply)
>   return -1;
> diff --git a/libmpathcmd/mpath_cmd.h b/libmpathcmd/mpath_cmd.h
> index 7293d91..de366a8 100644
> --- a/libmpathcmd/mpath_cmd.h
> +++ b/libmpathcmd/mpath_cmd.h
> @@ -26,8 +26,6 @@ extern "C" {
>  
>  #define DEFAULT_SOCKET   "/org/kernel/linux/storage/multipathd"
>  #define DEFAULT_REPLY_TIMEOUT1000
> -#define MAX_REPLY_LEN65536
> -
>  
>  /*
>   * DESCRIPTION:
> diff --git a/libmpathpersist/mpath_updatepr.c 
> b/libmpathpersist/mpath_updatepr.c
> index 9ff4b30..2504b6e 100644
> --- a/libmpathpersist/mpath_updatepr.c
> +++ b/libmpathpersist/mpath_updatepr.c
> @@ -7,13 +7,9 @@
>  #include 
>  #include 
>  #include 
> -#include 
> -#include 
> -#include 
>  #include 
>  #include "debug.h"
>  #include "mpath_cmd.h"
> -#include "uxsock.h"
>  #include "memory.h"
>  
>  unsigned long mem_allocated;/* Total memory used in Bytes */
> @@ -33,12 +29,12 @@ int update_prflag(char * arg1, char * arg2, int noisy)
>  
>   snprintf(str,sizeof(str),"map %s %s", arg1, arg2);
>   condlog (2, "%s: pr flag message=%s", arg1, str);
> - if (send_packet(fd, str) != 0) {
> + if (mpath_send_cmd(fd, str) != 0) {
>   condlog(2, "%s: message=%s send error=%d", arg1, str, errno);
>   mpath_disconnect(fd);
>   return -2;
>

Re: [dm-devel] [PATCH V7 2/4] multipath-tools: New way to limit the IPC command length.

2016-08-12 Thread Bart Van Assche 

On 08/12/2016 05:12 AM, Gris Ge wrote:

* Moved write_all() to file.h of libmultipath as all usage of
  write_all() is against file rather than socket.


This sounds very wrong. As far as I can see write_all() is used to send 
data over a Unix socket. A Unix socket is not a file. Additionally, 
using write_all() to write to a file does not make sense. Please use 
write() to write to a file.


Bart.

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

[dm-devel] [PATCH V7 2/4] multipath-tools: New way to limit the IPC command length.

2016-08-12 Thread Gris Ge 
Problem:

mpath_recv_reply() return -EINVAL on command 'show maps json' with 2k paths.

Root cause:

Commit 174e717d351789a3cb29e1417f8e910baabcdb16 introduced the
limitation on max bytes(65535) of reply string from multipathd.
With 2k paths(1k mpaths) simulated by scsi_debug, the 'show maps json'
requires 1633217 bytes which trigged the EINVAL error.

Fix:
* Remove the limitation of MAX_REPLY_LEN in libmpathcmd.

* Remove uxsock.h from libmultipath, changed all non-daemon usage to
  libmpathcmd instead.

* Rename send_packet() to send_packet_daemon_only() which is
  dedicated for multipathd socket listener.

* Rename recv_packet() to recv_packet_daemon_only() which is
  dedicate for multipathd socket listener.

* Enforce limitation(255) of IPC command string in
  recv_packet_daemon_only().

* Removed unused read_all() from uxsock.h.

* Moved write_all() to file.h of libmultipath as all usage of
  write_all() is against file rather than socket.

Changes caused by patch:

* Data sent from IPC client(including multipathd -k) to multipathd
  is limited to 255 bytes maximum. This prevent malicious IPC client
  send something like 'fake command' to daemon which
  lead daemon to allocate a big chunk of memory.

* Due to the removal of uxsock.h from libmultipath, all IPC connection
  except (multipathd daemon) should use libmpathcmd instead.

Signed-off-by: Gris Ge 
---
 libmpathcmd/mpath_cmd.c   |  2 -
 libmpathcmd/mpath_cmd.h   |  2 -
 libmpathpersist/mpath_updatepr.c  |  8 +---
 libmultipath/Makefile |  2 +-
 libmultipath/alias.c  |  1 -
 libmultipath/configure.c  |  5 +-
 libmultipath/file.c   | 24 +-
 libmultipath/file.h   |  1 +
 libmultipath/uxsock.h |  6 ---
 libmultipath/wwids.c  |  1 -
 multipath/main.c  |  1 -
 multipathd/Makefile   |  2 +-
 multipathd/uxclnt.c   | 16 +++
 multipathd/uxlsnr.c   | 10 ++--
 {libmultipath => multipathd}/uxsock.c | 89 +++
 multipathd/uxsock.h   |  8 
 16 files changed, 55 insertions(+), 123 deletions(-)
 delete mode 100644 libmultipath/uxsock.h
 rename {libmultipath => multipathd}/uxsock.c (57%)
 create mode 100644 multipathd/uxsock.h

diff --git a/libmpathcmd/mpath_cmd.c b/libmpathcmd/mpath_cmd.c
index 0daaf53..b400038 100644
--- a/libmpathcmd/mpath_cmd.c
+++ b/libmpathcmd/mpath_cmd.c
@@ -156,8 +156,6 @@ int mpath_recv_reply(int fd, char **reply, unsigned int 
timeout)
len = mpath_recv_reply_len(fd, timeout);
if (len <= 0)
return len;
-   if (len > MAX_REPLY_LEN)
-   return -EINVAL;
*reply = malloc(len);
if (!*reply)
return -1;
diff --git a/libmpathcmd/mpath_cmd.h b/libmpathcmd/mpath_cmd.h
index 7293d91..de366a8 100644
--- a/libmpathcmd/mpath_cmd.h
+++ b/libmpathcmd/mpath_cmd.h
@@ -26,8 +26,6 @@ extern "C" {
 
 #define DEFAULT_SOCKET "/org/kernel/linux/storage/multipathd"
 #define DEFAULT_REPLY_TIMEOUT  1000
-#define MAX_REPLY_LEN  65536
-
 
 /*
  * DESCRIPTION:
diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index 9ff4b30..2504b6e 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -7,13 +7,9 @@
 #include 
 #include 
 #include 
-#include 
-#include 
-#include 
 #include 
 #include "debug.h"
 #include "mpath_cmd.h"
-#include "uxsock.h"
 #include "memory.h"
 
 unsigned long mem_allocated;/* Total memory used in Bytes */
@@ -33,12 +29,12 @@ int update_prflag(char * arg1, char * arg2, int noisy)
 
snprintf(str,sizeof(str),"map %s %s", arg1, arg2);
condlog (2, "%s: pr flag message=%s", arg1, str);
-   if (send_packet(fd, str) != 0) {
+   if (mpath_send_cmd(fd, str) != 0) {
condlog(2, "%s: message=%s send error=%d", arg1, str, errno);
mpath_disconnect(fd);
return -2;
}
-   ret = recv_packet(fd, , DEFAULT_REPLY_TIMEOUT);
+   ret = mpath_recv_reply(fd, , DEFAULT_REPLY_TIMEOUT);
if (ret < 0) {
condlog(2, "%s: message=%s recv error=%d", arg1, str, errno);
ret = -2;
diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index e44397b..9550b3e 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -21,7 +21,7 @@ OBJS = memory.o parser.o vector.o devmapper.o callout.o \
hwtable.o blacklist.o util.o dmparser.o config.o \
structs.o discovery.o propsel.o dict.o \
pgpolicies.o debug.o defaults.o uevent.o \
-   switchgroup.o uxsock.o print.o alias.o log_pthread.o \
+   switchgroup.o print.o alias.o log_pthread.o \
log.o configure.o structs_vec.o sysfs.o