[{"id":1765238,"web_url":"http://patchwork.ozlabs.org/comment/1765238/","msgid":"<20170908110247.GS2399@orbyte.nwl.cc>","list_archive_url":null,"date":"2017-09-08T11:02:47","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":4285,"url":"http://patchwork.ozlabs.org/api/people/4285/","name":"Phil Sutter","email":"phil@nwl.cc"},"content":"Hi Hangbin,\n\nOn Fri, Sep 08, 2017 at 06:14:56PM +0800, Hangbin Liu wrote:\n[...]\n> diff --git a/lib/libnetlink.c b/lib/libnetlink.c\n> index be7ac86..37cfb5a 100644\n> --- a/lib/libnetlink.c\n> +++ b/lib/libnetlink.c\n> @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,\n> \t}\n> }\n> \n> +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)\n> +{\n> +\tstruct iovec *iov;\n> +\tint len = -1, buf_len = 32768;\n> +\tchar *buffer = *buf;\n\nIsn't it possible to make 'buffer' static instead of the two 'buf'\nvariables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have\nonly a single buffer which is shared between both functions instead of\ntwo which are independently allocated.\n\n> +\n> +\tint flag = MSG_PEEK | MSG_TRUNC;\n> +\n> +\tif (buffer == NULL)\n> +re_malloc:\n> +\t\tbuffer = malloc(buf_len);\n\nI think using realloc() here is more appropriate since there is no need\nto free the buffer in beforehand and calling realloc(NULL, len) is\nequivalent to calling malloc(len). I think 'realloc' is also a better\nname for the goto label.\n\n> +\tif (buffer == NULL) {\n> +\t\tfprintf(stderr, \"malloc error: no enough buffer\\n\");\n\nMinor typo here: s/no/not/\n\n> +\t\treturn -1;\n\nReturn -ENOMEM?\n\n> +\t}\n> +\n> +\tiov = msg->msg_iov;\n> +\tiov->iov_base = buffer;\n> +\tiov->iov_len = buf_len;\n> +\n> +re_recv:\n\nJust call this 'recv'? (Not really important though.)\n\n> +\tlen = recvmsg(fd, msg, flag);\n> +\n> +\tif (len < 0) {\n> +\t\tif (errno == EINTR || errno == EAGAIN)\n> +\t\t\treturn 0;\n\nInstead of returning 0 (which makes callers retry), goto re_recv?\n\n> +\t\tfprintf(stderr, \"netlink receive error %s (%d)\\n\",\n> +\t\t\tstrerror(errno), errno);\n> +\t\treturn len;\n> +\t}\n> +\n> +\tif (len == 0) {\n> +\t\tfprintf(stderr, \"EOF on netlink\\n\");\n> +\t\treturn -1;\n\nReturn -ENODATA here? (Initially I though about -EOF, but EOF is -1 so\nthat would be incorrect).\n\n> +\t}\n> +\n> +\tif (len > buf_len) {\n> +\t\tfree(buffer);\n\nIf you use realloc() above, this can be dropped.\n\n> +\t\tbuf_len = len;\n\nFor this to work you have to make buf_len static too, otherwise you will\nunnecessarily reallocate the buffer. Oh, and that also requires the\nsingle buffer (as pointed out above) because you will otherwise use a\ncommon buf_len for both static buffers passed to this function.\n\n> +\t\tflag = 0;\n> +\t\tgoto re_malloc;\n> +\t}\n> +\n> +\tif (flag != 0) {\n> +\t\tflag = 0;\n> +\t\tgoto re_recv;\n> +\t}\n> +\n> +\t*buf = buffer;\n> +\treturn len;\n> +}\n> +\n> int rtnl_dump_filter_l(struct rtnl_handle *rth,\n> \t\t const struct rtnl_dump_filter_arg *arg)\n> {\n> @@ -413,31 +466,20 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,\n> \t\t.msg_iov = &iov,\n> \t\t.msg_iovlen = 1,\n> \t};\n> -\tchar buf[32768];\n> +\tstatic char *buf = NULL;\n\nIf you keep the static buffer in rtnl_recvmsg(), there is no need to\nassign NULL here.\n\n> \tint dump_intr = 0;\n> \n> -\tiov.iov_base = buf;\n> \twhile (1) {\n> \t\tint status;\n> \t\tconst struct rtnl_dump_filter_arg *a;\n> \t\tint found_done = 0;\n> \t\tint msglen = 0;\n> \n> -\t\tiov.iov_len = sizeof(buf);\n> -\t\tstatus = recvmsg(rth->fd, &msg, 0);\n> -\n> -\t\tif (status < 0) {\n> -\t\t\tif (errno == EINTR || errno == EAGAIN)\n> -\t\t\t\tcontinue;\n> -\t\t\tfprintf(stderr, \"netlink receive error %s (%d)\\n\",\n> -\t\t\t\tstrerror(errno), errno);\n> -\t\t\treturn -1;\n> -\t\t}\n> -\n> -\t\tif (status == 0) {\n> -\t\t\tfprintf(stderr, \"EOF on netlink\\n\");\n> -\t\t\treturn -1;\n> -\t\t}\n> +\t\tstatus = rtnl_recvmsg(rth->fd, &msg, &buf);\n> +\t\tif (status < 0)\n> +\t\t\treturn status;\n> +\t\telse if (status == 0)\n> +\t\t\tcontinue;\n\nWhen retrying inside rtnl_recvmsg(), it won't return 0 anymore. I\nbelieve the whole 'while (1)' loop could go away then.\n\nEverything noted for rtnl_dump_filter_l() applies to __rtnl_talk() as\nwell.\n\nThanks, Phil","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpZDH2mjmz9s81\n\tfor ;\n\tFri, 8 Sep 2017 21:02:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752872AbdIHLCv (ORCPT );\n\tFri, 8 Sep 2017 07:02:51 -0400","from orbyte.nwl.cc ([151.80.46.58]:36892 \"EHLO orbyte.nwl.cc\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752463AbdIHLCu (ORCPT );\n\tFri, 8 Sep 2017 07:02:50 -0400","from n0-1 by orbyte.nwl.cc with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dqH3f-0007vf-Fx; Fri, 08 Sep 2017 13:02:47 +0200"],"Date":"Fri, 8 Sep 2017 13:02:47 +0200","From":"Phil Sutter ","To":"Hangbin Liu ","Cc":"netdev@vger.kernel.org, Stephen Hemminger ,\n\tMichal Kubecek ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170908110247.GS2399@orbyte.nwl.cc>","Mail-Followup-To":"Phil Sutter ,\n\tHangbin Liu , netdev@vger.kernel.org,\n\tStephen Hemminger ,\n\tMichal Kubecek ","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>","User-Agent":"Mutt/1.7.2 (2016-11-26)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765299,"web_url":"http://patchwork.ozlabs.org/comment/1765299/","msgid":"<20170908123237.xzbvypljnsgitngr@unicorn.suse.cz>","list_archive_url":null,"date":"2017-09-08T12:32:38","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":11892,"url":"http://patchwork.ozlabs.org/api/people/11892/","name":"Michal Kubecek","email":"mkubecek@suse.cz"},"content":"On Fri, Sep 08, 2017 at 01:02:47PM +0200, Phil Sutter wrote:\n> Hi Hangbin,\n> \n> On Fri, Sep 08, 2017 at 06:14:56PM +0800, Hangbin Liu wrote:\n> [...]\n> > diff --git a/lib/libnetlink.c b/lib/libnetlink.c\n> > index be7ac86..37cfb5a 100644\n> > --- a/lib/libnetlink.c\n> > +++ b/lib/libnetlink.c\n> > @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,\n> > \t}\n> > }\n> > \n> > +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)\n> > +{\n> > +\tstruct iovec *iov;\n> > +\tint len = -1, buf_len = 32768;\n> > +\tchar *buffer = *buf;\n> \n> Isn't it possible to make 'buffer' static instead of the two 'buf'\n> variables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have\n> only a single buffer which is shared between both functions instead of\n> two which are independently allocated.\n\nDo we have to worry about reentrancy? Only arpd is multithreaded in\niproute2 but there might be also other users of libnetlink.\n\n> > +\n> > +\tint flag = MSG_PEEK | MSG_TRUNC;\n> > +\n> > +\tif (buffer == NULL)\n> > +re_malloc:\n> > +\t\tbuffer = malloc(buf_len);\n> \n> I think using realloc() here is more appropriate since there is no need\n> to free the buffer in beforehand and calling realloc(NULL, len) is\n> equivalent to calling malloc(len). I think 'realloc' is also a better\n> name for the goto label.\n> \n> > +\tif (buffer == NULL) {\n> > +\t\tfprintf(stderr, \"malloc error: no enough buffer\\n\");\n> \n> Minor typo here: s/no/not/\n> \n> > +\t\treturn -1;\n> \n> Return -ENOMEM?\n\nHm... the only caller of rtnl_dump_filter_l only checks if the return\nvalue is negative. Even worse, at least some of the functions calling\n__rtnl_talk() check errno (or use perror()) instead, even if it's not\nalways preserved. That's something for a wider cleanup, I would say.\n\n> \n> > +\t}\n> > +\n> > +\tiov = msg->msg_iov;\n> > +\tiov->iov_base = buffer;\n> > +\tiov->iov_len = buf_len;\n> > +\n> > +re_recv:\n> \n> Just call this 'recv'? (Not really important though.)\n> \n> > +\tlen = recvmsg(fd, msg, flag);\n> > +\n> > +\tif (len < 0) {\n> > +\t\tif (errno == EINTR || errno == EAGAIN)\n> > +\t\t\treturn 0;\n> \n> Instead of returning 0 (which makes callers retry), goto re_recv?\n> \n> > +\t\tfprintf(stderr, \"netlink receive error %s (%d)\\n\",\n> > +\t\t\tstrerror(errno), errno);\n> > +\t\treturn len;\n> > +\t}\n> > +\n> > +\tif (len == 0) {\n> > +\t\tfprintf(stderr, \"EOF on netlink\\n\");\n> > +\t\treturn -1;\n> \n> Return -ENODATA here? (Initially I though about -EOF, but EOF is -1 so\n> that would be incorrect).\n> \n> > +\t}\n> > +\n> > +\tif (len > buf_len) {\n> > +\t\tfree(buffer);\n> \n> If you use realloc() above, this can be dropped.\n> \n> > +\t\tbuf_len = len;\n> \n> For this to work you have to make buf_len static too, otherwise you will\n> unnecessarily reallocate the buffer. Oh, and that also requires the\n> single buffer (as pointed out above) because you will otherwise use a\n> common buf_len for both static buffers passed to this function.\n> \n> > +\t\tflag = 0;\n> > +\t\tgoto re_malloc;\n> > +\t}\n> > +\n> > +\tif (flag != 0) {\n> > +\t\tflag = 0;\n> > +\t\tgoto re_recv;\n> > +\t}\n> > +\n> > +\t*buf = buffer;\n> > +\treturn len;\n> > +}\n> > +\n> > int rtnl_dump_filter_l(struct rtnl_handle *rth,\n> > \t\t const struct rtnl_dump_filter_arg *arg)\n> > {\n> > @@ -413,31 +466,20 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,\n> > \t\t.msg_iov = &iov,\n> > \t\t.msg_iovlen = 1,\n> > \t};\n> > -\tchar buf[32768];\n> > +\tstatic char *buf = NULL;\n> \n> If you keep the static buffer in rtnl_recvmsg(), there is no need to\n> assign NULL here.\n> \n> > \tint dump_intr = 0;\n> > \n> > -\tiov.iov_base = buf;\n> > \twhile (1) {\n> > \t\tint status;\n> > \t\tconst struct rtnl_dump_filter_arg *a;\n> > \t\tint found_done = 0;\n> > \t\tint msglen = 0;\n> > \n> > -\t\tiov.iov_len = sizeof(buf);\n> > -\t\tstatus = recvmsg(rth->fd, &msg, 0);\n> > -\n> > -\t\tif (status < 0) {\n> > -\t\t\tif (errno == EINTR || errno == EAGAIN)\n> > -\t\t\t\tcontinue;\n> > -\t\t\tfprintf(stderr, \"netlink receive error %s (%d)\\n\",\n> > -\t\t\t\tstrerror(errno), errno);\n> > -\t\t\treturn -1;\n> > -\t\t}\n> > -\n> > -\t\tif (status == 0) {\n> > -\t\t\tfprintf(stderr, \"EOF on netlink\\n\");\n> > -\t\t\treturn -1;\n> > -\t\t}\n> > +\t\tstatus = rtnl_recvmsg(rth->fd, &msg, &buf);\n> > +\t\tif (status < 0)\n> > +\t\t\treturn status;\n> > +\t\telse if (status == 0)\n> > +\t\t\tcontinue;\n> \n> When retrying inside rtnl_recvmsg(), it won't return 0 anymore. I\n> believe the whole 'while (1)' loop could go away then.\n\nDoesn't this loop also handle the response divided into multiple\npackets?\n\nMichal Kubecek","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpcNw1S4tz9t4t\n\tfor ;\n\tFri, 8 Sep 2017 22:40:31 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755686AbdIHMcl (ORCPT );\n\tFri, 8 Sep 2017 08:32:41 -0400","from mx2.suse.de ([195.135.220.15]:41788 \"EHLO mx1.suse.de\"\n\trhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP\n\tid S1753645AbdIHMck (ORCPT );\n\tFri, 8 Sep 2017 08:32:40 -0400","from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx1.suse.de (Postfix) with ESMTP id CC0BAAEA1;\n\tFri, 8 Sep 2017 12:32:38 +0000 (UTC)","by unicorn.suse.cz (Postfix, from userid 1000)\n\tid 63868A0F3A; Fri, 8 Sep 2017 14:32:38 +0200 (CEST)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Fri, 8 Sep 2017 14:32:38 +0200","From":"Michal Kubecek ","To":"Phil Sutter , Hangbin Liu ,\n\tnetdev@vger.kernel.org, Stephen Hemminger ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170908123237.xzbvypljnsgitngr@unicorn.suse.cz>","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>\n\t<20170908110247.GS2399@orbyte.nwl.cc>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170908110247.GS2399@orbyte.nwl.cc>","User-Agent":"NeoMutt/20170421 (1.8.2)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765391,"web_url":"http://patchwork.ozlabs.org/comment/1765391/","msgid":"<20170908140131.GU5465@leo.usersys.redhat.com>","list_archive_url":null,"date":"2017-09-08T14:01:31","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":15639,"url":"http://patchwork.ozlabs.org/api/people/15639/","name":"Hangbin Liu","email":"liuhangbin@gmail.com"},"content":"Hi Phil,\n\nThanks for the comments, see replies bellow.\n\nOn Fri, Sep 08, 2017 at 01:02:47PM +0200, Phil Sutter wrote:\n> Hi Hangbin,\n> \n> On Fri, Sep 08, 2017 at 06:14:56PM +0800, Hangbin Liu wrote:\n> [...]\n> > diff --git a/lib/libnetlink.c b/lib/libnetlink.c\n> > index be7ac86..37cfb5a 100644\n> > --- a/lib/libnetlink.c\n> > +++ b/lib/libnetlink.c\n> > @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,\n> > \t}\n> > }\n> > \n> > +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)\n> > +{\n> > +\tstruct iovec *iov;\n> > +\tint len = -1, buf_len = 32768;\n> > +\tchar *buffer = *buf;\n> \n> Isn't it possible to make 'buffer' static instead of the two 'buf'\n> variables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have\n> only a single buffer which is shared between both functions instead of\n> two which are independently allocated.\n\nI was also thinking of this before. But in function ipaddrlabel_flush()\n\n\tif (rtnl_dump_filter(&rth, flush_addrlabel, NULL) < 0)\n\nIt will cal rtnl_dump_filter_l() first via\nrtnl_dump_filter() -> rtnl_dump_filter_nc() -> rtnl_dump_filter_l().\n\nThen call rtnl_talk() later via call back\na->filter(&nladdr, h, a->arg1) -> flush_addrlabel() -> rtnl_talk()\n\nSo if we only use one static buffer in rtnl_recvmsg(). Then it will be written\nat lease twice.\n\nThe path looks like bellow in function rtnl_dump_filter_l()\n\n\twhile (1) {\n\t\tstatus = rtnl_recvmsg(rth->fd, &msg, &buf);\t<== write buf\n\n\t\tfor (a = arg; a->filter; a++) {\n\t\t\tstruct nlmsghdr *h = (struct nlmsghdr *)buf;\t<== assign buf to h\n\n\t\t\twhile (NLMSG_OK(h, msglen)) {\n\n\t\t\t\tif (!rth->dump_fp) {\n\t\t\t\t\terr = a->filter(&nladdr, h, a->arg1);\t<== buf changed via rtnl_talk()\n\t\t\t\t}\n\n\t\t\t\th = NLMSG_NEXT(h, msglen);\t<== so h will also be changed\n\t\t\t}\n\t\t}\n\t}\n\nThat's why I have to use two static buffers.\n> \n> > +\n> > +\tint flag = MSG_PEEK | MSG_TRUNC;\n> > +\n> > +\tif (buffer == NULL)\n> > +re_malloc:\n> > +\t\tbuffer = malloc(buf_len);\n> \n> I think using realloc() here is more appropriate since there is no need\n> to free the buffer in beforehand and calling realloc(NULL, len) is\n> equivalent to calling malloc(len). I think 'realloc' is also a better\n> name for the goto label.\n\nGood idea.\n> \n> > +\tif (buffer == NULL) {\n> > +\t\tfprintf(stderr, \"malloc error: no enough buffer\\n\");\n> \n> Minor typo here: s/no/not/\n> \n> > +\t\treturn -1;\n> \n> Return -ENOMEM?\n> \n> > +\t}\n> > +\n> > +\tiov = msg->msg_iov;\n> > +\tiov->iov_base = buffer;\n> > +\tiov->iov_len = buf_len;\n> > +\n> > +re_recv:\n> \n> Just call this 'recv'? (Not really important though.)\n> \n> > +\tlen = recvmsg(fd, msg, flag);\n> > +\n> > +\tif (len < 0) {\n> > +\t\tif (errno == EINTR || errno == EAGAIN)\n> > +\t\t\treturn 0;\n> \n> Instead of returning 0 (which makes callers retry), goto re_recv?\n\nYes, will fix this.\n> \n> > +\t\tfprintf(stderr, \"netlink receive error %s (%d)\\n\",\n> > +\t\t\tstrerror(errno), errno);\n> > +\t\treturn len;\n> > +\t}\n> > +\n> > +\tif (len == 0) {\n> > +\t\tfprintf(stderr, \"EOF on netlink\\n\");\n> > +\t\treturn -1;\n> \n> Return -ENODATA here? (Initially I though about -EOF, but EOF is -1 so\n> that would be incorrect).\n> \n> > +\t}\n> > +\n> > +\tif (len > buf_len) {\n> > +\t\tfree(buffer);\n> \n> If you use realloc() above, this can be dropped.\n\nYes.\n> \n> > +\t\tbuf_len = len;\n> \n> For this to work you have to make buf_len static too, otherwise you will\n> unnecessarily reallocate the buffer. Oh, and that also requires the\n> single buffer (as pointed out above) because you will otherwise use a\n> common buf_len for both static buffers passed to this function.\n\nSince we have to use two static bufffers. So how about check like\n\n\tif (len > strlen(buffer))\n\n> \n> > +\t\tflag = 0;\n> > +\t\tgoto re_malloc;\n> > +\t}\n> > +\n> > +\tif (flag != 0) {\n> > +\t\tflag = 0;\n> > +\t\tgoto re_recv;\n> > +\t}\n> > +\n> > +\t*buf = buffer;\n> > +\treturn len;\n> > +}\n> > +\n> > int rtnl_dump_filter_l(struct rtnl_handle *rth,\n> > \t\t const struct rtnl_dump_filter_arg *arg)\n> > {\n> > @@ -413,31 +466,20 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,\n> > \t\t.msg_iov = &iov,\n> > \t\t.msg_iovlen = 1,\n> > \t};\n> > -\tchar buf[32768];\n> > +\tstatic char *buf = NULL;\n> \n> If you keep the static buffer in rtnl_recvmsg(), there is no need to\n> assign NULL here.\n> \n> > \tint dump_intr = 0;\n> > \n> > -\tiov.iov_base = buf;\n> > \twhile (1) {\n> > \t\tint status;\n> > \t\tconst struct rtnl_dump_filter_arg *a;\n> > \t\tint found_done = 0;\n> > \t\tint msglen = 0;\n> > \n> > -\t\tiov.iov_len = sizeof(buf);\n> > -\t\tstatus = recvmsg(rth->fd, &msg, 0);\n> > -\n> > -\t\tif (status < 0) {\n> > -\t\t\tif (errno == EINTR || errno == EAGAIN)\n> > -\t\t\t\tcontinue;\n> > -\t\t\tfprintf(stderr, \"netlink receive error %s (%d)\\n\",\n> > -\t\t\t\tstrerror(errno), errno);\n> > -\t\t\treturn -1;\n> > -\t\t}\n> > -\n> > -\t\tif (status == 0) {\n> > -\t\t\tfprintf(stderr, \"EOF on netlink\\n\");\n> > -\t\t\treturn -1;\n> > -\t\t}\n> > +\t\tstatus = rtnl_recvmsg(rth->fd, &msg, &buf);\n> > +\t\tif (status < 0)\n> > +\t\t\treturn status;\n> > +\t\telse if (status == 0)\n> > +\t\t\tcontinue;\n> \n> When retrying inside rtnl_recvmsg(), it won't return 0 anymore. I\n> believe the whole 'while (1)' loop could go away then.\n> \n\nLike Michal said, there may have multi netlink packets?\n\nThanks\nHangbin","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"HQmvlONy\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpfBf0Jglz9s7G\n\tfor ;\n\tSat, 9 Sep 2017 00:01:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753173AbdIHOBn (ORCPT );\n\tFri, 8 Sep 2017 10:01:43 -0400","from mail-pf0-f193.google.com ([209.85.192.193]:33398 \"EHLO\n\tmail-pf0-f193.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751166AbdIHOBm (ORCPT\n\t); Fri, 8 Sep 2017 10:01:42 -0400","by mail-pf0-f193.google.com with SMTP id h4so1476945pfk.0\n\tfor ; Fri, 08 Sep 2017 07:01:42 -0700 (PDT)","from leo.usersys.redhat.com ([209.132.188.80])\n\tby smtp.gmail.com with ESMTPSA id\n\tv126sm4033693pfv.113.2017.09.08.07.01.39\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 08 Sep 2017 07:01:41 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=date:from:to:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=i4p0jAP7IjEsxm1DKvLQQ2Ta5KFloUK0+0ZQ6aarhQg=;\n\tb=HQmvlONyT1h03tSWqxIBZ4XxI+9xgMxVnsFZ1D6uWTXnp8Ei7bYBVdQ47Ut5cz2Df/\n\tYyJ2qzF4MP9ZmuStTYskJV4rTcIOXT0Mge6OuHpgeVbxHBw/vW8/UOu7hzp72HYkZNUT\n\tgYvTWKZq5MQtGqRN/Jglr8Xx7SvcpeD7S7Q4spA0xzBeohv1kdJX+D3P26oOztupLSnR\n\tkO5isSf75dzTS54eefSCJ/jxjDnQPXswlrMwmwe2lYt0MYBSU4zhPnGJPiCF2IYC17DG\n\t650yEcKJJe20alce/tFUWMcT1GNQVzFHF2ZY6dl1Ub6WyA3faXPj869E8akw6z76aUCA\n\tSQ1Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=i4p0jAP7IjEsxm1DKvLQQ2Ta5KFloUK0+0ZQ6aarhQg=;\n\tb=o1aBSrPv5zPfRrXINObNSFmc/NJ0rcL1STHPuSnAi7wEH9BCx9a3SrLq35Nsj7ELk2\n\tM0cC+Q1kYaF9+FL60fpTGfV06OxVPTvMMwsh4hQSmyB55feBdtnKYRxedbG9GcWNBMKm\n\tWU0JiiGxQ9CIB9LW/1nKpU9n1YugvsqUJWUYv5vb+XWjhKm3k/BGvkoNgIdewKmX0gHL\n\t/u91wRG4vWbUcE1bn1a4o0TbPIdprJ5cXiB4nVltANz74aHkkdqSJ8tZy3GG8Gyaiip4\n\thYh4vZnxT5QOGXcKqd6AKROHiY09J5MFTo7/ZkRUfj6O6Z8usZli+nTWSIXkB9VpPoDF\n\tKGgQ==","X-Gm-Message-State":"AHPjjUjrmvwhpg4EQoGMNrCQKGeWEddcK6odZgUBxTeE2LbJ9MwUjau0\n\tGGppc6LsHCHJWg==","X-Google-Smtp-Source":"ADKCNb7z7tWVCy+pHYFZ4pczmUHGVA6rXYBCMiXqX0zmtr0TxR1OA0+/6dR97tdRzcyO+vbP7yRtzA==","X-Received":"by 10.98.37.196 with SMTP id l187mr3179014pfl.161.1504879301992; \n\tFri, 08 Sep 2017 07:01:41 -0700 (PDT)","Date":"Fri, 8 Sep 2017 22:01:31 +0800","From":"Hangbin Liu ","To":"Phil Sutter , netdev@vger.kernel.org,\n\tStephen Hemminger ,\n\tMichal Kubecek ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170908140131.GU5465@leo.usersys.redhat.com>","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>\n\t<20170908110247.GS2399@orbyte.nwl.cc>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170908110247.GS2399@orbyte.nwl.cc>","User-Agent":"Mutt/1.7.0 (2016-08-17)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765430,"web_url":"http://patchwork.ozlabs.org/comment/1765430/","msgid":"<20170908145113.GA30028@orbyte.nwl.cc>","list_archive_url":null,"date":"2017-09-08T14:51:13","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":4285,"url":"http://patchwork.ozlabs.org/api/people/4285/","name":"Phil Sutter","email":"phil@nwl.cc"},"content":"Hi,\n\nOn Fri, Sep 08, 2017 at 10:01:31PM +0800, Hangbin Liu wrote:\n[...]\n> > > diff --git a/lib/libnetlink.c b/lib/libnetlink.c\n> > > index be7ac86..37cfb5a 100644\n> > > --- a/lib/libnetlink.c\n> > > +++ b/lib/libnetlink.c\n> > > @@ -402,6 +402,59 @@ static void rtnl_dump_error(const struct rtnl_handle *rth,\n> > > \t}\n> > > }\n> > > \n> > > +static int rtnl_recvmsg(int fd, struct msghdr *msg, char **buf)\n> > > +{\n> > > +\tstruct iovec *iov;\n> > > +\tint len = -1, buf_len = 32768;\n> > > +\tchar *buffer = *buf;\n> > \n> > Isn't it possible to make 'buffer' static instead of the two 'buf'\n> > variables in rtnl_dump_filter_l() and __rtnl_talk()? Then we would have\n> > only a single buffer which is shared between both functions instead of\n> > two which are independently allocated.\n> \n> I was also thinking of this before. But in function ipaddrlabel_flush()\n> \n> \tif (rtnl_dump_filter(&rth, flush_addrlabel, NULL) < 0)\n> \n> It will cal rtnl_dump_filter_l() first via\n> rtnl_dump_filter() -> rtnl_dump_filter_nc() -> rtnl_dump_filter_l().\n> \n> Then call rtnl_talk() later via call back\n> a->filter(&nladdr, h, a->arg1) -> flush_addrlabel() -> rtnl_talk()\n> \n> So if we only use one static buffer in rtnl_recvmsg(). Then it will be written\n> at lease twice.\n\nOh yes, in that case we really can't have a single buffer.\n\n[...]\n> > > +\t\tbuf_len = len;\n> > \n> > For this to work you have to make buf_len static too, otherwise you will\n> > unnecessarily reallocate the buffer. Oh, and that also requires the\n> > single buffer (as pointed out above) because you will otherwise use a\n> > common buf_len for both static buffers passed to this function.\n> \n> Since we have to use two static bufffers. So how about check like\n> \n> \tif (len > strlen(buffer))\n\nI don't think that will work. We are reusing the buffer and it contains\nbinary data, so a NUL byte may appear anywhere. I fear you will have to\nchange rtnl_recvmsg() to accept a buflen parameter which callers have to\ndefine statically together with the buffer pointer.\n\nRegarding Michal's concern about reentrancy, maybe we should go into a\ndifferent direction and make rtnl_recvmsg() return a newly allocated\nbuffer which the caller has to free.\n\n[...]\n> > When retrying inside rtnl_recvmsg(), it won't return 0 anymore. I\n> > believe the whole 'while (1)' loop could go away then.\n> > \n> \n> Like Michal said, there may have multi netlink packets?\n\nAh yes, I missed that.\n\nThanks, Phil","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpgHq1QFyz9s82\n\tfor ;\n\tSat, 9 Sep 2017 00:51:19 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755997AbdIHOvR (ORCPT );\n\tFri, 8 Sep 2017 10:51:17 -0400","from orbyte.nwl.cc ([151.80.46.58]:37334 \"EHLO orbyte.nwl.cc\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1755906AbdIHOvQ (ORCPT );\n\tFri, 8 Sep 2017 10:51:16 -0400","from n0-1 by orbyte.nwl.cc with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dqKcj-0002DJ-Na; Fri, 08 Sep 2017 16:51:13 +0200"],"Date":"Fri, 8 Sep 2017 16:51:13 +0200","From":"Phil Sutter ","To":"Hangbin Liu ","Cc":"netdev@vger.kernel.org, Stephen Hemminger ,\n\tMichal Kubecek ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170908145113.GA30028@orbyte.nwl.cc>","Mail-Followup-To":"Phil Sutter ,\n\tHangbin Liu , netdev@vger.kernel.org,\n\tStephen Hemminger ,\n\tMichal Kubecek ","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>\n\t<20170908110247.GS2399@orbyte.nwl.cc>\n\t<20170908140131.GU5465@leo.usersys.redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170908140131.GU5465@leo.usersys.redhat.com>","User-Agent":"Mutt/1.7.2 (2016-11-26)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766094,"web_url":"http://patchwork.ozlabs.org/comment/1766094/","msgid":"<20170911071955.GZ5465@leo.usersys.redhat.com>","list_archive_url":null,"date":"2017-09-11T07:19:55","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":15639,"url":"http://patchwork.ozlabs.org/api/people/15639/","name":"Hangbin Liu","email":"liuhangbin@gmail.com"},"content":"Hi Phil and Michal,\nOn Fri, Sep 08, 2017 at 04:51:13PM +0200, Phil Sutter wrote:\n> Hi,\n> \n> On Fri, Sep 08, 2017 at 10:01:31PM +0800, Hangbin Liu wrote:\n> [...]\n> > Since we have to use two static bufffers. So how about check like\n> > \n> > \tif (len > strlen(buffer))\n> \n> I don't think that will work. We are reusing the buffer and it contains\n> binary data, so a NUL byte may appear anywhere. I fear you will have to\n> change rtnl_recvmsg() to accept a buflen parameter which callers have to\n> define statically together with the buffer pointer.\n\nOK\n> \n> Regarding Michal's concern about reentrancy, maybe we should go into a\n> different direction and make rtnl_recvmsg() return a newly allocated\n> buffer which the caller has to free.\n\nHmm... But we could not free the buf in __rtnl_talk(). Because in\n__rtnl_talk() we assign the answer with the buf address and return to caller.\n\n\tfor (h = (struct nlmsghdr *)buf; status >= sizeof(*h); ) {\n\t\t[...]\n\t\tif (answer) {\n\t\t\t*answer= h;\n\t\t\treturn 0;\n\t\t}\n\t}\n\nAnd the caller will keep use it in later code. Since there are plenty of\nfunctions called rtnl_talk. I think it would be much more complex to free\nthe buffer every time.\n\n\nHi Michal,\n\nWould you like to tell me more about your concern with reentrancy? It's looks\narpd doesn't call rtnl_talk() or rtnl_dump_filter_l().\n\nThanks\nHangbin","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"b9TLUf9F\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrK7t3NDvz9s4q\n\tfor ;\n\tMon, 11 Sep 2017 17:20:10 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751026AbdIKHUH (ORCPT );\n\tMon, 11 Sep 2017 03:20:07 -0400","from mail-pg0-f50.google.com ([74.125.83.50]:38115 \"EHLO\n\tmail-pg0-f50.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751007AbdIKHUG (ORCPT\n\t); Mon, 11 Sep 2017 03:20:06 -0400","by mail-pg0-f50.google.com with SMTP id v66so13868773pgb.5\n\tfor ; Mon, 11 Sep 2017 00:20:06 -0700 (PDT)","from leo.usersys.redhat.com ([209.132.188.80])\n\tby smtp.gmail.com with ESMTPSA id\n\tu77sm14629367pfk.87.2017.09.11.00.20.03\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tMon, 11 Sep 2017 00:20:05 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=date:from:to:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=FgYUtkr0oztsu7ne6jRZjH6YfFD/eu0CFYK2Ud6ViDk=;\n\tb=b9TLUf9Fh4vc6W7XEZ6sXZjYTqr+Yi2HmoJehUbrT3jFmXdiQ9qNVA5UlMrmFufqV/\n\taRqpBpT/AvpbfHBOeWAxTBfj7fn/1XrheDgXxFIoGD6KZ7mam7ji5f1dxaOw01YvGKoK\n\tXwOk7Yf+q7vtWIAlF0luw6loZguO6bWqlgd0tYq3sM+38GPeEBFjtK+T1w39uvPB9+oN\n\tvjXVDjxdy1gfPU9h/XW+pHVqy/+ACMCcbCqwo3iIhnv7e8RFmQejbuKLc3xbVUbWh0pH\n\t40aZnzi511DWZlXVxeou8/wBeRMQEcsi2dCbAwvOQE07wJElhhgdUNyPUbYhAfypBGSR\n\t1h4g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=FgYUtkr0oztsu7ne6jRZjH6YfFD/eu0CFYK2Ud6ViDk=;\n\tb=JfurXCws6Lfzw7uwZoTTDhRzwA7vTDnX4HvLvlBAUFo9jxlUoNoKIJTgggPdMYswol\n\toIMKpfQfY2OAHrKeiHl5fjKOa5p2/fjh4yQJdwhxVnUx4wFMj2mBf8YLBG9O2LSjgvbf\n\teTHqlj050I5vYtMzZMQiyBGtM6jP9AF/EU1oFRW7Opy4dWh1cwO+l08m7zN98KaKqOHF\n\tTnzMpvzvR6oOL/TyqZmRVIv8gzphBjrfk9uA7uZn3G8v44lgMv8P59Mpt+2SL6WCeOKK\n\tanxGpKHJNCDjPBcDwXdzTlTmNa2L/DCX/wN2bvRirygXfdOox7ugH8+Hbe1Vu9FFxGtI\n\tnZhQ==","X-Gm-Message-State":"AHPjjUiG5TyjbmbpRSu9rNIAKQ8swuR5iZ4gYmrQHnapbOfUQ4FPp6Hx\n\tzR5RgalYa2lueq40","X-Google-Smtp-Source":"ADKCNb5bwXTBPMOGWaQ5YrEkRnFyWs5SKBelszgOyo/Dao626RuhvDh3v52MME4MMaJyEwlioKN7OA==","X-Received":"by 10.84.238.139 with SMTP id v11mr12602810plk.344.1505114405904;\n\tMon, 11 Sep 2017 00:20:05 -0700 (PDT)","Date":"Mon, 11 Sep 2017 15:19:55 +0800","From":"Hangbin Liu ","To":"Phil Sutter , netdev@vger.kernel.org,\n\tStephen Hemminger ,\n\tMichal Kubecek ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170911071955.GZ5465@leo.usersys.redhat.com>","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>\n\t<20170908110247.GS2399@orbyte.nwl.cc>\n\t<20170908140131.GU5465@leo.usersys.redhat.com>\n\t<20170908145113.GA30028@orbyte.nwl.cc>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170908145113.GA30028@orbyte.nwl.cc>","User-Agent":"Mutt/1.7.0 (2016-08-17)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766770,"web_url":"http://patchwork.ozlabs.org/comment/1766770/","msgid":"<20170912084748.m4wz6kxfclc2mkxd@unicorn.suse.cz>","list_archive_url":null,"date":"2017-09-12T08:47:48","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":11892,"url":"http://patchwork.ozlabs.org/api/people/11892/","name":"Michal Kubecek","email":"mkubecek@suse.cz"},"content":"On Mon, Sep 11, 2017 at 03:19:55PM +0800, Hangbin Liu wrote:\n> On Fri, Sep 08, 2017 at 04:51:13PM +0200, Phil Sutter wrote:\n> > Regarding Michal's concern about reentrancy, maybe we should go into a\n> > different direction and make rtnl_recvmsg() return a newly allocated\n> > buffer which the caller has to free.\n> \n> Hmm... But we could not free the buf in __rtnl_talk(). Because in\n> __rtnl_talk() we assign the answer with the buf address and return to caller.\n> \n> \tfor (h = (struct nlmsghdr *)buf; status >= sizeof(*h); ) {\n> \t\t[...]\n> \t\tif (answer) {\n> \t\t\t*answer= h;\n> \t\t\treturn 0;\n> \t\t}\n> \t}\n> \n> And the caller will keep use it in later code. Since there are plenty of\n> functions called rtnl_talk. I think it would be much more complex to free\n> the buffer every time.\n> \n> \n> Hi Michal,\n> \n> Would you like to tell me more about your concern with reentrancy? It's looks\n> arpd doesn't call rtnl_talk() or rtnl_dump_filter_l().\n\nI checked again and arpd indeed isn't a problem. It doesn't seem to call\nany of the two functions (directly or indirectly) and while it's linked\nwith \"-lpthread\", it's not really multithreaded.\n\nBut my concern was rather about other potential users of libnetlink\n(i.e. those which are not part of iproute2). I must admit, though, that\nI'm not sure if libnetlink code is reentrant as of now. (And people are\ndiscouraged from using it in its own manual page.)\n\nThat being said, I still like Phil's idea for a different reason. While\ninvestigating the issue with \"ip link show dev eth ...\" which led me to\ncommit 6599162b958e (\"iplink: check for message truncation in\niplink_get()\"), I quickly peeked at some other callers of rtnl_talk()\nand I'm afraid there may be others which wouldn't handle truncated\nmessage correctly. I assume the maxlen argument was always chosen to be\nsufficient for any expected messages but as the example of iplink_get()\nshows, messages returned by kernel my grow over time.\n\nThat's why I like the idea of __rtnl_talk() returning a pointer to newly\nallocated buffer (of sufficient size) rather than copying the response\ninto a buffer provided by caller and potentially truncating it.\n\nMichal Kubecek","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrz2f5JKVz9s0g\n\tfor ;\n\tTue, 12 Sep 2017 18:47:54 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751292AbdILIrw (ORCPT );\n\tTue, 12 Sep 2017 04:47:52 -0400","from mx2.suse.de ([195.135.220.15]:60340 \"EHLO mx1.suse.de\"\n\trhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP\n\tid S1751251AbdILIrv (ORCPT );\n\tTue, 12 Sep 2017 04:47:51 -0400","from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx1.suse.de (Postfix) with ESMTP id E2715AB22;\n\tTue, 12 Sep 2017 08:47:49 +0000 (UTC)","by unicorn.suse.cz (Postfix, from userid 1000)\n\tid 9B1C7A0F37; Tue, 12 Sep 2017 10:47:48 +0200 (CEST)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Tue, 12 Sep 2017 10:47:48 +0200","From":"Michal Kubecek ","To":"Hangbin Liu ","Cc":"Phil Sutter , netdev@vger.kernel.org,\n\tStephen Hemminger ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170912084748.m4wz6kxfclc2mkxd@unicorn.suse.cz>","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>\n\t<20170908110247.GS2399@orbyte.nwl.cc>\n\t<20170908140131.GU5465@leo.usersys.redhat.com>\n\t<20170908145113.GA30028@orbyte.nwl.cc>\n\t<20170911071955.GZ5465@leo.usersys.redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170911071955.GZ5465@leo.usersys.redhat.com>","User-Agent":"NeoMutt/20170421 (1.8.2)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1766786,"web_url":"http://patchwork.ozlabs.org/comment/1766786/","msgid":"<20170912090926.j7vwhq6a57c5d6wx@unicorn.suse.cz>","list_archive_url":null,"date":"2017-09-12T09:09:26","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":11892,"url":"http://patchwork.ozlabs.org/api/people/11892/","name":"Michal Kubecek","email":"mkubecek@suse.cz"},"content":"On Tue, Sep 12, 2017 at 10:47:48AM +0200, Michal Kubecek wrote:\n> On Mon, Sep 11, 2017 at 03:19:55PM +0800, Hangbin Liu wrote:\n> > On Fri, Sep 08, 2017 at 04:51:13PM +0200, Phil Sutter wrote:\n> > > Regarding Michal's concern about reentrancy, maybe we should go into a\n> > > different direction and make rtnl_recvmsg() return a newly allocated\n> > > buffer which the caller has to free.\n> > \n> > Hmm... But we could not free the buf in __rtnl_talk(). Because in\n> > __rtnl_talk() we assign the answer with the buf address and return to caller.\n> > \n> > \tfor (h = (struct nlmsghdr *)buf; status >= sizeof(*h); ) {\n> > \t\t[...]\n> > \t\tif (answer) {\n> > \t\t\t*answer= h;\n> > \t\t\treturn 0;\n> > \t\t}\n> > \t}\n> > \n> > And the caller will keep use it in later code. Since there are plenty of\n> > functions called rtnl_talk. I think it would be much more complex to free\n> > the buffer every time.\n> > \n> > \n> > Hi Michal,\n> > \n> > Would you like to tell me more about your concern with reentrancy? It's looks\n> > arpd doesn't call rtnl_talk() or rtnl_dump_filter_l().\n> \n> I checked again and arpd indeed isn't a problem. It doesn't seem to call\n> any of the two functions (directly or indirectly) and while it's linked\n> with \"-lpthread\", it's not really multithreaded.\n> \n> But my concern was rather about other potential users of libnetlink\n> (i.e. those which are not part of iproute2). I must admit, though, that\n> I'm not sure if libnetlink code is reentrant as of now. (And people are\n> discouraged from using it in its own manual page.)\n> \n> That being said, I still like Phil's idea for a different reason. While\n> investigating the issue with \"ip link show dev eth ...\" which led me to\n> commit 6599162b958e (\"iplink: check for message truncation in\n> iplink_get()\"), I quickly peeked at some other callers of rtnl_talk()\n> and I'm afraid there may be others which wouldn't handle truncated\n> message correctly. I assume the maxlen argument was always chosen to be\n> sufficient for any expected messages but as the example of iplink_get()\n> shows, messages returned by kernel my grow over time.\n> \n> That's why I like the idea of __rtnl_talk() returning a pointer to newly\n> allocated buffer (of sufficient size) rather than copying the response\n> into a buffer provided by caller and potentially truncating it.\n\nI'm sorry, I managed to forget that your patch 2 does already address\nthis problem. But the fact that any caller must keep in mind that he\nmust not call the same function again until the previous response is no\nlonger needed still feels like a trap. It's something you need to keep\nin mind (where \"you\" in fact means any future contributor) and it's\neasy to forget. That's why I prefer the reentrant functions like\nstrerror_r() or localtime_r() even in code which is not intended to be\nmultithreaded. Getting an object which is \"mine\" to do with whatever\nI want until I no longer need it feels like a cleaner interface to me.\n\nMichal Kubecek","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrzWf5Vz8z9s83\n\tfor ;\n\tTue, 12 Sep 2017 19:09:34 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751810AbdILJJa (ORCPT );\n\tTue, 12 Sep 2017 05:09:30 -0400","from mx2.suse.de ([195.135.220.15]:33454 \"EHLO mx1.suse.de\"\n\trhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP\n\tid S1751472AbdILJJ2 (ORCPT );\n\tTue, 12 Sep 2017 05:09:28 -0400","from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx1.suse.de (Postfix) with ESMTP id 24569AB22;\n\tTue, 12 Sep 2017 09:09:27 +0000 (UTC)","by unicorn.suse.cz (Postfix, from userid 1000)\n\tid BE5CEA0F37; Tue, 12 Sep 2017 11:09:26 +0200 (CEST)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Tue, 12 Sep 2017 11:09:26 +0200","From":"Michal Kubecek ","To":"Hangbin Liu ","Cc":"Phil Sutter , netdev@vger.kernel.org,\n\tStephen Hemminger ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170912090926.j7vwhq6a57c5d6wx@unicorn.suse.cz>","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>\n\t<20170908110247.GS2399@orbyte.nwl.cc>\n\t<20170908140131.GU5465@leo.usersys.redhat.com>\n\t<20170908145113.GA30028@orbyte.nwl.cc>\n\t<20170911071955.GZ5465@leo.usersys.redhat.com>\n\t<20170912084748.m4wz6kxfclc2mkxd@unicorn.suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170912084748.m4wz6kxfclc2mkxd@unicorn.suse.cz>","User-Agent":"NeoMutt/20170421 (1.8.2)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1767711,"web_url":"http://patchwork.ozlabs.org/comment/1767711/","msgid":"<20170913092657.GK5465@leo.usersys.redhat.com>","list_archive_url":null,"date":"2017-09-13T09:26:58","subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","submitter":{"id":15639,"url":"http://patchwork.ozlabs.org/api/people/15639/","name":"Hangbin Liu","email":"liuhangbin@gmail.com"},"content":"Hi Michal,\n\nThanks a lot for all your explains. Phil has helped update the patch to support\nreturn a newly allocated buffer. I will post it soon.\n\nThanks\nHangbin\nOn Tue, Sep 12, 2017 at 11:09:26AM +0200, Michal Kubecek wrote:\n> > \n> > I checked again and arpd indeed isn't a problem. It doesn't seem to call\n> > any of the two functions (directly or indirectly) and while it's linked\n> > with \"-lpthread\", it's not really multithreaded.\n> > \n> > But my concern was rather about other potential users of libnetlink\n> > (i.e. those which are not part of iproute2). I must admit, though, that\n> > I'm not sure if libnetlink code is reentrant as of now. (And people are\n> > discouraged from using it in its own manual page.)\n> > \n> > That being said, I still like Phil's idea for a different reason. While\n> > investigating the issue with \"ip link show dev eth ...\" which led me to\n> > commit 6599162b958e (\"iplink: check for message truncation in\n> > iplink_get()\"), I quickly peeked at some other callers of rtnl_talk()\n> > and I'm afraid there may be others which wouldn't handle truncated\n> > message correctly. I assume the maxlen argument was always chosen to be\n> > sufficient for any expected messages but as the example of iplink_get()\n> > shows, messages returned by kernel my grow over time.\n> > \n> > That's why I like the idea of __rtnl_talk() returning a pointer to newly\n> > allocated buffer (of sufficient size) rather than copying the response\n> > into a buffer provided by caller and potentially truncating it.\n> \n> I'm sorry, I managed to forget that your patch 2 does already address\n> this problem. But the fact that any caller must keep in mind that he\n> must not call the same function again until the previous response is no\n> longer needed still feels like a trap. It's something you need to keep\n> in mind (where \"you\" in fact means any future contributor) and it's\n> easy to forget. That's why I prefer the reentrant functions like\n> strerror_r() or localtime_r() even in code which is not intended to be\n> multithreaded. Getting an object which is \"mine\" to do with whatever\n> I want until I no longer need it feels like a cleaner interface to me.\n> \n> Michal Kubecek\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"EFAaX9r6\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsbsY0qsbz9sNr\n\tfor ;\n\tWed, 13 Sep 2017 19:27:12 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752133AbdIMJ1L (ORCPT );\n\tWed, 13 Sep 2017 05:27:11 -0400","from mail-pf0-f194.google.com ([209.85.192.194]:38233 \"EHLO\n\tmail-pf0-f194.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751545AbdIMJ1J (ORCPT\n\t); Wed, 13 Sep 2017 05:27:09 -0400","by mail-pf0-f194.google.com with SMTP id q76so7289718pfq.5\n\tfor ; Wed, 13 Sep 2017 02:27:09 -0700 (PDT)","from leo.usersys.redhat.com ([209.132.188.80])\n\tby smtp.gmail.com with ESMTPSA id\n\tq12sm21503978pgs.47.2017.09.13.02.27.06\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 13 Sep 2017 02:27:08 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=quavaX69u+Aifxk6vJP+i+a5Md881wld3C2clcwgE5Y=;\n\tb=EFAaX9r6pPk+Aqt5m7hWlPJmovYXfcshoeugqTtyRyY4rAorTflWsk4e56tU8JFt+q\n\tygY1hUNZhqqZ5RQ0rGn8DWrP6ehx2nzT3cmhiEAxf8fjyytOyC2+R0P8k2UdS+in085g\n\tcLCdTDEhvwv00KSUhfh631fTKvNUBh4RQncdYb9VW3GDW9I7m8d1N0Wa9BcQvbdL5G8s\n\t1zC1wpRxigN2n/IstCdmHjCXkZAeDgWQRHwIZh4mAf7rSwDYEHzbRHMxX7yewZ304Zk0\n\tHz1kAcPKzw1NpBXrl3UR7iVP0uPfWwDfU2MF9Uwap4gnnhtb4LdKSMONWPzmCyI+ofJF\n\t5Msg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=quavaX69u+Aifxk6vJP+i+a5Md881wld3C2clcwgE5Y=;\n\tb=QSxuEeFXNFFU/osEUR/s0xlmWhDN5boGCepbUJxs0mLhlK1WqjZfx2W5G6TDQzn0GF\n\t/jnB1zum22uiPoi5ybRnzQWFGh2qDJixLUQTQCdcYasPEZhYQWPosiZfRQgPbrYJMFoM\n\tedMBKST5TJoPTeKgTIf2exdmjYDDuU2vmsB9wAR375CVfXEey6jbFlSA6zG+pa0TxDo5\n\t60XAaVqayQq/fWXzf8fPOprp3tqK+OyVoX4kGSL3aHLxzI8mWOPYHJHMBPt+A5DABATH\n\t3CB1YHBwghQxe4AxwvE0lfrygs42SdJgHeZiwJTVxIg4LCkZS8BQYlRtEOliWycbBHdy\n\tn6MQ==","X-Gm-Message-State":"AHPjjUj11NMqPD7+Bt8BHZmvVUSxcOjTE0CaHQlNV6pboA9S7p9kZXMe\n\tCjkv9pFOqFVa6A==","X-Google-Smtp-Source":"ADKCNb4nrhv4I0uZvv5D7pmGsfRVmgodLF2zydsndm5X009trSYdmRvxIgegN13xqyEojO+cDqK9YQ==","X-Received":"by 10.98.14.213 with SMTP id 82mr18162245pfo.274.1505294828851; \n\tWed, 13 Sep 2017 02:27:08 -0700 (PDT)","Date":"Wed, 13 Sep 2017 17:26:58 +0800","From":"Hangbin Liu ","To":"Michal Kubecek ","Cc":"Phil Sutter , netdev@vger.kernel.org,\n\tStephen Hemminger ","Subject":"Re: [PATCH iproute2 1/2] lib/libnetlink: re malloc buff if size is\n\tnot enough","Message-ID":"<20170913092657.GK5465@leo.usersys.redhat.com>","References":"<1504865697-27274-1-git-send-email-liuhangbin@gmail.com>\n\t<1504865697-27274-2-git-send-email-liuhangbin@gmail.com>\n\t<20170908110247.GS2399@orbyte.nwl.cc>\n\t<20170908140131.GU5465@leo.usersys.redhat.com>\n\t<20170908145113.GA30028@orbyte.nwl.cc>\n\t<20170911071955.GZ5465@leo.usersys.redhat.com>\n\t<20170912084748.m4wz6kxfclc2mkxd@unicorn.suse.cz>\n\t<20170912090926.j7vwhq6a57c5d6wx@unicorn.suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170912090926.j7vwhq6a57c5d6wx@unicorn.suse.cz>","User-Agent":"Mutt/1.7.0 (2016-08-17)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]