| Message ID | 20210913040442.2627-1-yajun.deng@linux.dev (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d7807a9adf4856171f8441f13078c33941df48ab |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Revert "ipv4: fix memory leaks in ip_cmsg_send() callers" | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 9 this patch: 9 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 89 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 9 this patch: 9 |
| netdev/header_inline | success | Link |
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Mon, 13 Sep 2021 12:04:42 +0800 you wrote: > This reverts commit 919483096bfe75dda338e98d56da91a263746a0a. > > There is only when ip_options_get() return zero need to free. > It already called kfree() when return error. > > Fixes: 919483096bfe ("ipv4: fix memory leaks in ip_cmsg_send() callers") > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > > [...] Here is the summary with links: - Revert "ipv4: fix memory leaks in ip_cmsg_send() callers" https://git.kernel.org/netdev/net/c/d7807a9adf48 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Sun, Sep 12, 2021 at 9:04 PM Yajun Deng <yajun.deng@linux.dev> wrote: > > This reverts commit 919483096bfe75dda338e98d56da91a263746a0a. > > There is only when ip_options_get() return zero need to free. > It already called kfree() when return error. > > Fixes: 919483096bfe ("ipv4: fix memory leaks in ip_cmsg_send() callers") > Signed-off-by: Yajun Deng <yajun.deng@linux.dev> > --- I do not think this is a valid patch, not sure why David has merged so soon before us reviewing it ? You are bringing back the memory leaks. ip_cmsg_send() can loop over multiple cmsghdr() If IP_RETOPTS has been successful, but following cmsghdr generates an error, we do not free ipc.ok If IP_RETOPTS is not successful, we have freed the allocated temporary space, not the one currently in ipc.opt. Can you share what your exact finding was, perhaps a syzbot repro ??? Thanks.
September 14, 2021 12:15 AM, "Eric Dumazet" <edumazet@google.com> wrote: > On Sun, Sep 12, 2021 at 9:04 PM Yajun Deng <yajun.deng@linux.dev> wrote: > >> This reverts commit 919483096bfe75dda338e98d56da91a263746a0a. >> >> There is only when ip_options_get() return zero need to free. >> It already called kfree() when return error. >> >> Fixes: 919483096bfe ("ipv4: fix memory leaks in ip_cmsg_send() callers") >> Signed-off-by: Yajun Deng <yajun.deng@linux.dev> >> --- > > I do not think this is a valid patch, not sure why David has merged so > soon before us reviewing it ? > > You are bringing back the memory leaks. > > ip_cmsg_send() can loop over multiple cmsghdr() > Yes, I forgot the loop, it was my mistake. > If IP_RETOPTS has been successful, but following cmsghdr generates an error, > we do not free ipc.ok > > If IP_RETOPTS is not successful, we have freed the allocated temporary space, > not the one currently in ipc.opt. > > Can you share what your exact finding was, perhaps a syzbot repro ??? > > Thanks.
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c index b297bb28556e..7cef9987ab4a 100644 --- a/net/ipv4/ip_sockglue.c +++ b/net/ipv4/ip_sockglue.c @@ -279,7 +279,7 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc, case IP_RETOPTS: err = cmsg->cmsg_len - sizeof(struct cmsghdr); - /* Our caller is responsible for freeing ipc->opt */ + /* Our caller is responsible for freeing ipc->opt when err = 0 */ err = ip_options_get(net, &ipc->opt, KERNEL_SOCKPTR(CMSG_DATA(cmsg)), err < 40 ? err : 40); diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c index 1e44a43acfe2..c588f9f2f46c 100644 --- a/net/ipv4/ping.c +++ b/net/ipv4/ping.c @@ -727,10 +727,9 @@ static int ping_v4_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) if (msg->msg_controllen) { err = ip_cmsg_send(sk, msg, &ipc, false); - if (unlikely(err)) { - kfree(ipc.opt); + if (unlikely(err)) return err; - } + if (ipc.opt) free = 1; } diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c index bb446e60cf58..1c98063a3ae8 100644 --- a/net/ipv4/raw.c +++ b/net/ipv4/raw.c @@ -562,10 +562,9 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) if (msg->msg_controllen) { err = ip_cmsg_send(sk, msg, &ipc, false); - if (unlikely(err)) { - kfree(ipc.opt); + if (unlikely(err)) goto out; - } + if (ipc.opt) free = 1; } diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 8851c9463b4b..d5f5981d7a43 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1122,10 +1122,9 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) if (err > 0) err = ip_cmsg_send(sk, msg, &ipc, sk->sk_family == AF_INET6); - if (unlikely(err < 0)) { - kfree(ipc.opt); + if (unlikely(err < 0)) return err; - } + if (ipc.opt) free = 1; connected = 0;
This reverts commit 919483096bfe75dda338e98d56da91a263746a0a. There is only when ip_options_get() return zero need to free. It already called kfree() when return error. Fixes: 919483096bfe ("ipv4: fix memory leaks in ip_cmsg_send() callers") Signed-off-by: Yajun Deng <yajun.deng@linux.dev> --- net/ipv4/ip_sockglue.c | 2 +- net/ipv4/ping.c | 5 ++--- net/ipv4/raw.c | 5 ++--- net/ipv4/udp.c | 5 ++--- 4 files changed, 7 insertions(+), 10 deletions(-)