[dm-devel] [PATCH V7 2/4] multipath-tools: New way to limit the IPC command length.
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, &reply, DEFAULT_REPLY_TIMEOUT); + ret = mpath_recv_reply(fd, &reply, 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 prio.o ch
Re: [dm-devel] [PATCH V7 2/4] multipath-tools: New way to limit the IPC command length.
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
Re: [dm-devel] [PATCH V7 2/4] multipath-tools: New way to limit the IPC command length.
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; > } > - ret = re