| Message ID | 1681820060-12044-1-git-send-email-wangyunjian@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netlink: Fix memory leak dpif_netlink_open(). | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Tue, Apr 18, 2023 at 2:14 PM Yunjian Wang via dev <ovs-dev@openvswitch.org> wrote: > > Reported by Address Sanitizer. > > Indirect leak of 1024 byte(s) in 1 object(s) allocated from: > #0 0x7fe09d3bfe70 in __interceptor_malloc (/usr/lib64/libasan.so.4+0xe0e70) > #1 0x8759be in xmalloc__ lib/util.c:140 > #2 0x875a9a in xmalloc lib/util.c:175 > #3 0x7ba0d2 in ofpbuf_init lib/ofpbuf.c:141 > #4 0x7ba1d6 in ofpbuf_new lib/ofpbuf.c:169 > #5 0x9057f9 in nl_sock_transact lib/netlink-socket.c:1113 > #6 0x907a7e in nl_transact lib/netlink-socket.c:1817 > #7 0x8b5abe in dpif_netlink_dp_transact lib/dpif-netlink.c:5007 > #8 0x89a6b5 in dpif_netlink_open lib/dpif-netlink.c:398 > #9 0x5de16f in do_open lib/dpif.c:348 > #10 0x5de69a in dpif_open lib/dpif.c:393 > #11 0x5de71f in dpif_create_and_open lib/dpif.c:419 > #12 0x47b918 in open_dpif_backer ofproto/ofproto-dpif.c:764 > #13 0x483e4a in construct ofproto/ofproto-dpif.c:1658 > #14 0x441644 in ofproto_create ofproto/ofproto.c:556 > #15 0x40ba5a in bridge_reconfigure vswitchd/bridge.c:885 > #16 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > #17 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > #18 0x7fe09cc03c86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) > > Fixes: b841e3cd4a28 ("dpif-netlink: Fix feature negotiation for older kernels.") > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- > lib/dpif-netlink.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index de0711277..82801640c 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -428,6 +428,7 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name, > error = open_dpif(&dp, dpifp); > dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); > } else { > + ofpbuf_delete(buf); > VLOG_INFO("Kernel does not correctly support feature negotiation. " > "Using standard features."); > dp_request.cmd = OVS_DP_CMD_SET; The fix looks good to me. I wonder though if we should pass any ofpbuf at all, as we don't care about the reply from the kernel for this case. Something like: $ git diff diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index de07112778..60bd39643c 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -395,7 +395,7 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name, dp_request.user_features |= OVS_DP_F_UNALIGNED; dp_request.user_features |= OVS_DP_F_VPORT_PIDS; dp_request.user_features |= OVS_DP_F_UNSUPPORTED; - error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); + error = dpif_netlink_dp_transact(&dp_request, NULL, NULL); if (error) { /* The Open vSwitch kernel module has two modes for dispatching * upcalls: per-vport and per-cpu. WDYT?
> -----Original Message----- > From: David Marchand [mailto:david.marchand@redhat.com] > Sent: Tuesday, April 18, 2023 8:57 PM > To: wangyunjian <wangyunjian@huawei.com> > Cc: dev@openvswitch.org; i.maximets@ovn.org; luyicai <luyicai@huawei.com> > Subject: Re: [ovs-dev] [PATCH] dpif-netlink: Fix memory leak dpif_netlink_open(). > > On Tue, Apr 18, 2023 at 2:14 PM Yunjian Wang via dev > <ovs-dev@openvswitch.org> wrote: > > > > Reported by Address Sanitizer. > > > > Indirect leak of 1024 byte(s) in 1 object(s) allocated from: > > #0 0x7fe09d3bfe70 in __interceptor_malloc > (/usr/lib64/libasan.so.4+0xe0e70) > > #1 0x8759be in xmalloc__ lib/util.c:140 > > #2 0x875a9a in xmalloc lib/util.c:175 > > #3 0x7ba0d2 in ofpbuf_init lib/ofpbuf.c:141 > > #4 0x7ba1d6 in ofpbuf_new lib/ofpbuf.c:169 > > #5 0x9057f9 in nl_sock_transact lib/netlink-socket.c:1113 > > #6 0x907a7e in nl_transact lib/netlink-socket.c:1817 > > #7 0x8b5abe in dpif_netlink_dp_transact lib/dpif-netlink.c:5007 > > #8 0x89a6b5 in dpif_netlink_open lib/dpif-netlink.c:398 > > #9 0x5de16f in do_open lib/dpif.c:348 > > #10 0x5de69a in dpif_open lib/dpif.c:393 > > #11 0x5de71f in dpif_create_and_open lib/dpif.c:419 > > #12 0x47b918 in open_dpif_backer ofproto/ofproto-dpif.c:764 > > #13 0x483e4a in construct ofproto/ofproto-dpif.c:1658 > > #14 0x441644 in ofproto_create ofproto/ofproto.c:556 > > #15 0x40ba5a in bridge_reconfigure vswitchd/bridge.c:885 > > #16 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 > > #17 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 > > #18 0x7fe09cc03c86 in __libc_start_main > > (/usr/lib64/libc.so.6+0x25c86) > > > > Fixes: b841e3cd4a28 ("dpif-netlink: Fix feature negotiation for older > > kernels.") > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > --- > > lib/dpif-netlink.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index > > de0711277..82801640c 100644 > > --- a/lib/dpif-netlink.c > > +++ b/lib/dpif-netlink.c > > @@ -428,6 +428,7 @@ dpif_netlink_open(const struct dpif_class *class > OVS_UNUSED, const char *name, > > error = open_dpif(&dp, dpifp); > > dpif_netlink_set_features(*dpifp, > OVS_DP_F_TC_RECIRC_SHARING); > > } else { > > + ofpbuf_delete(buf); > > VLOG_INFO("Kernel does not correctly support feature > negotiation. " > > "Using standard features."); > > dp_request.cmd = OVS_DP_CMD_SET; > > The fix looks good to me. > > I wonder though if we should pass any ofpbuf at all, as we don't care about the > reply from the kernel for this case. > Something like: > > $ git diff > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index de07112778..60bd39643c > 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -395,7 +395,7 @@ dpif_netlink_open(const struct dpif_class *class > OVS_UNUSED, const char *name, > dp_request.user_features |= OVS_DP_F_UNALIGNED; > dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > dp_request.user_features |= OVS_DP_F_UNSUPPORTED; > - error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > + error = dpif_netlink_dp_transact(&dp_request, NULL, NULL); > if (error) { > /* The Open vSwitch kernel module has two modes for dispatching > * upcalls: per-vport and per-cpu. > > WDYT? Currently, the 'buf' will check whether it is NULL in the dpif_netlink_dp_transact(). And it will be used in dpif_netlink_dp_from_ofpbuf(). Thanks, Yunjian > > > -- > David Marchand
On Tue, Apr 18, 2023 at 3:16 PM wangyunjian <wangyunjian@huawei.com> wrote: > > I wonder though if we should pass any ofpbuf at all, as we don't care about the > > reply from the kernel for this case. > > Something like: > > > > $ git diff > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index de07112778..60bd39643c > > 100644 > > --- a/lib/dpif-netlink.c > > +++ b/lib/dpif-netlink.c > > @@ -395,7 +395,7 @@ dpif_netlink_open(const struct dpif_class *class > > OVS_UNUSED, const char *name, > > dp_request.user_features |= OVS_DP_F_UNALIGNED; > > dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > > dp_request.user_features |= OVS_DP_F_UNSUPPORTED; > > - error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > > + error = dpif_netlink_dp_transact(&dp_request, NULL, NULL); > > if (error) { > > /* The Open vSwitch kernel module has two modes for dispatching > > * upcalls: per-vport and per-cpu. > > > > WDYT? > > Currently, the 'buf' will check whether it is NULL in the dpif_netlink_dp_transact(). > And it will be used in dpif_netlink_dp_from_ofpbuf(). - Looking at dpif_netlink_dp_transact() comments. /* Executes 'request' in the kernel datapath. If the command fails, returns a * positive errno value. Otherwise, if 'reply' and 'bufp' are null, returns 0 * without doing anything else. If 'reply' and 'bufp' are nonnull, then the * result of the command is expected to be of the same form, which is decoded * and stored in '*reply' and '*bufp'. The caller must free '*bufp' when the * reply is no longer needed ('reply' will contain pointers into '*bufp'). */ static int dpif_netlink_dp_transact(const struct dpif_netlink_dp *request, struct dpif_netlink_dp *reply, struct ofpbuf **bufp) I understand that dpif_netlink_dp_transact() makes it possible for a caller to simply ignore the reply from the kernel by passing both 'reply' and 'bufp' as NULL. - In the specific call to dpif_netlink_dp_transact() (line 398) in dpif_netlink_open(), the 'dp' content is not considered in the branch when no error returned (starting line 430). Besides, 'dp' / 'buf' are overwritten later in this same branch, by sending a new netlink request (line 437). So if this code is not going to use 'dp' and 'buf' content, why not simply pass NULL/NULL?
> -----Original Message----- > From: David Marchand [mailto:david.marchand@redhat.com] > Sent: Wednesday, April 19, 2023 2:44 PM > To: wangyunjian <wangyunjian@huawei.com> > Cc: dev@openvswitch.org; i.maximets@ovn.org; luyicai <luyicai@huawei.com> > Subject: Re: [ovs-dev] [PATCH] dpif-netlink: Fix memory leak dpif_netlink_open(). > > On Tue, Apr 18, 2023 at 3:16 PM wangyunjian <wangyunjian@huawei.com> > wrote: > > > I wonder though if we should pass any ofpbuf at all, as we don't > > > care about the reply from the kernel for this case. > > > Something like: > > > > > > $ git diff > > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index > > > de07112778..60bd39643c > > > 100644 > > > --- a/lib/dpif-netlink.c > > > +++ b/lib/dpif-netlink.c > > > @@ -395,7 +395,7 @@ dpif_netlink_open(const struct dpif_class *class > > > OVS_UNUSED, const char *name, > > > dp_request.user_features |= OVS_DP_F_UNALIGNED; > > > dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > > > dp_request.user_features |= OVS_DP_F_UNSUPPORTED; > > > - error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > > > + error = dpif_netlink_dp_transact(&dp_request, NULL, NULL); > > > if (error) { > > > /* The Open vSwitch kernel module has two modes for > dispatching > > > * upcalls: per-vport and per-cpu. > > > > > > WDYT? > > > > Currently, the 'buf' will check whether it is NULL in the > dpif_netlink_dp_transact(). > > And it will be used in dpif_netlink_dp_from_ofpbuf(). > > - Looking at dpif_netlink_dp_transact() comments. > > /* Executes 'request' in the kernel datapath. If the command fails, returns a > * positive errno value. Otherwise, if 'reply' and 'bufp' are null, returns 0 > * without doing anything else. If 'reply' and 'bufp' are nonnull, then the > * result of the command is expected to be of the same form, which is decoded > * and stored in '*reply' and '*bufp'. The caller must free '*bufp' when the > * reply is no longer needed ('reply' will contain pointers into '*bufp'). */ static > int dpif_netlink_dp_transact(const struct dpif_netlink_dp *request, > struct dpif_netlink_dp *reply, struct ofpbuf > **bufp) > > I understand that dpif_netlink_dp_transact() makes it possible for a caller to > simply ignore the reply from the kernel by passing both 'reply' and 'bufp' as > NULL. > > > - In the specific call to dpif_netlink_dp_transact() (line 398) in > dpif_netlink_open(), the 'dp' content is not considered in the branch when no > error returned (starting line 430). > Besides, 'dp' / 'buf' are overwritten later in this same branch, by sending a new > netlink request (line 437). > > So if this code is not going to use 'dp' and 'buf' content, why not simply pass > NULL/NULL? You are right. I fix it on your suggestions. http://patchwork.ozlabs.org/project/openvswitch/patch/1681890598-18740-1-git-send-email-wangyunjian@huawei.com/ Thank you. Yunjian > > > -- > David Marchand
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index de0711277..82801640c 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -428,6 +428,7 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name, error = open_dpif(&dp, dpifp); dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); } else { + ofpbuf_delete(buf); VLOG_INFO("Kernel does not correctly support feature negotiation. " "Using standard features."); dp_request.cmd = OVS_DP_CMD_SET;
Reported by Address Sanitizer. Indirect leak of 1024 byte(s) in 1 object(s) allocated from: #0 0x7fe09d3bfe70 in __interceptor_malloc (/usr/lib64/libasan.so.4+0xe0e70) #1 0x8759be in xmalloc__ lib/util.c:140 #2 0x875a9a in xmalloc lib/util.c:175 #3 0x7ba0d2 in ofpbuf_init lib/ofpbuf.c:141 #4 0x7ba1d6 in ofpbuf_new lib/ofpbuf.c:169 #5 0x9057f9 in nl_sock_transact lib/netlink-socket.c:1113 #6 0x907a7e in nl_transact lib/netlink-socket.c:1817 #7 0x8b5abe in dpif_netlink_dp_transact lib/dpif-netlink.c:5007 #8 0x89a6b5 in dpif_netlink_open lib/dpif-netlink.c:398 #9 0x5de16f in do_open lib/dpif.c:348 #10 0x5de69a in dpif_open lib/dpif.c:393 #11 0x5de71f in dpif_create_and_open lib/dpif.c:419 #12 0x47b918 in open_dpif_backer ofproto/ofproto-dpif.c:764 #13 0x483e4a in construct ofproto/ofproto-dpif.c:1658 #14 0x441644 in ofproto_create ofproto/ofproto.c:556 #15 0x40ba5a in bridge_reconfigure vswitchd/bridge.c:885 #16 0x41f1a9 in bridge_run vswitchd/bridge.c:3313 #17 0x42d4fb in main vswitchd/ovs-vswitchd.c:132 #18 0x7fe09cc03c86 in __libc_start_main (/usr/lib64/libc.so.6+0x25c86) Fixes: b841e3cd4a28 ("dpif-netlink: Fix feature negotiation for older kernels.") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> --- lib/dpif-netlink.c | 1 + 1 file changed, 1 insertion(+)