On Tue, Jun 02, 2020 at 09:49:10PM -0700, Cong Wang wrote:
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 9f357aa22b94..bcbba0bef1c2 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -513,15 +513,58 @@ static void genl_family_rcv_msg_attrs_free(const struct
> genl_family *family,
> kfree(attrbuf);
> }
>
> -static int genl_lock_start(struct netlink_callback *cb)
> +struct genl_start_context {
> + const struct genl_family *family;
> + struct nlmsghdr *nlh;
> + struct netlink_ext_ack *extack;
> + const struct genl_ops *ops;
> + int hdrlen;
> +};
> +
> +static int genl_start(struct netlink_callback *cb)
> {
> - const struct genl_ops *ops = genl_dumpit_info(cb)->ops;
> + struct genl_start_context *ctx = cb->data;
> + const struct genl_ops *ops = ctx->ops;
> + struct genl_dumpit_info *info;
> + struct nlattr **attrs = NULL;
> int rc = 0;
>
> + if (ops->validate & GENL_DONT_VALIDATE_DUMP)
> + goto no_attrs;
> +
> + if (ctx->nlh->nlmsg_len < nlmsg_msg_size(ctx->hdrlen))
> + return -EINVAL;
> +
> + attrs = genl_family_rcv_msg_attrs_parse(ctx->family, ctx->nlh,
> ctx->extack,
> + ops, ctx->hdrlen,
> + GENL_DONT_VALIDATE_DUMP_STRICT,
> + true);
> + if (IS_ERR(attrs))
> + return PTR_ERR(attrs);
> +
> +no_attrs:
> + info = genl_dumpit_info_alloc();
> + if (!info) {
> + kfree(attrs);
> + return -ENOMEM;
> + }
> + info->family = ctx->family;
> + info->ops = ops;
> + info->attrs = attrs;
> +
> + cb->data = info;
> if (ops->start) {
> - genl_lock();
> + if (!ctx->family->parallel_ops)
> + genl_lock();
> rc = ops->start(cb);
> - genl_unlock();
> + if (!ctx->family->parallel_ops)
> + genl_unlock();
> + }
> +
> + if (rc) {
> + kfree(attrs);
> + genl_dumpit_info_free(info);
> + cb->data = NULL;
> }
> return rc;
> }
> @@ -548,7 +591,7 @@ static int genl_lock_done(struct netlink_callback *cb)
> rc = ops->done(cb);
> genl_unlock();
> }
> - genl_family_rcv_msg_attrs_free(info->family, info->attrs, true);
> + genl_family_rcv_msg_attrs_free(info->family, info->attrs, false);
Cong,
This seems to result in a memory leak because 'info->attrs' is never
freed in the non-parallel case.
Both the parallel and non-parallel code paths call genl_start() which
allocates the array, but the latter calls genl_lock_done() as its done()
callback which never frees it.
Can be reproduced as follows:
echo "10 1" > /sys/bus/netdevsim/new_device
devlink trap &> /dev/null
echo scan > /sys/kernel/debug/kmemleak
cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff88810f1ed000 (size 2048):
comm "devlink", pid 201, jiffies 4295606431 (age 35.858s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<00000000a7cb7530>] __kmalloc+0x1d6/0x3d0
[<000000001cb013e1>] genl_family_rcv_msg_attrs_parse+0x1f3/0x320
[<00000000b201bc93>] genl_start+0x1ab/0x5e0
[<00000000786e531e>] __netlink_dump_start+0x5b5/0x940
[<00000000a2332fcb>] genl_family_rcv_msg_dumpit+0x32e/0x3a0
[<00000000112052dd>] genl_rcv_msg+0x6d7/0xb40
[<000000005826e358>] netlink_rcv_skb+0x175/0x490
[<000000002c5f41ae>] genl_rcv+0x2d/0x40
[<00000000f0301e6d>] netlink_unicast+0x5d0/0x7f0
[<00000000a76a3934>] netlink_sendmsg+0x981/0xe90
[<000000001c478a6f>] __sys_sendto+0x2cd/0x450
[<0000000079d420b0>] __x64_sys_sendto+0xe6/0x1a0
[<000000004e535e4b>] do_syscall_64+0xc1/0x600
[<000000006e5dd3c4>] entry_SYSCALL_64_after_hwframe+0x49/0xb3
> genl_dumpit_info_free(info);
> return rc;
> }
> @@ -573,43 +616,23 @@ static int genl_family_rcv_msg_dumpit(const struct
> genl_family *family,
> const struct genl_ops *ops,
> int hdrlen, struct net *net)
> {
> - struct genl_dumpit_info *info;
> - struct nlattr **attrs = NULL;
> + struct genl_start_context ctx;
> int err;
>
> if (!ops->dumpit)
> return -EOPNOTSUPP;
>
> - if (ops->validate & GENL_DONT_VALIDATE_DUMP)
> - goto no_attrs;
> -
> - if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
> - return -EINVAL;
> -
> - attrs = genl_family_rcv_msg_attrs_parse(family, nlh, extack,
> - ops, hdrlen,
> - GENL_DONT_VALIDATE_DUMP_STRICT,
> - true);
> - if (IS_ERR(attrs))
> - return PTR_ERR(attrs);
> -
> -no_attrs:
> - /* Allocate dumpit info. It is going to be freed by done() callback. */
> - info = genl_dumpit_info_alloc();
> - if (!info) {
> - genl_family_rcv_msg_attrs_free(family, attrs, true);
> - return -ENOMEM;
> - }
> -
> - info->family = family;
> - info->ops = ops;
> - info->attrs = attrs;
> + ctx.family = family;
> + ctx.nlh = nlh;
> + ctx.extack = extack;
> + ctx.ops = ops;
> + ctx.hdrlen = hdrlen;
>
> if (!family->parallel_ops) {
> struct netlink_dump_control c = {
> .module = family->module,
> - .data = info,
> - .start = genl_lock_start,
> + .data = &ctx,
> + .start = genl_start,
> .dump = genl_lock_dumpit,
> .done = genl_lock_done,
> };
> @@ -617,12 +640,11 @@ static int genl_family_rcv_msg_dumpit(const struct
> genl_family *family,
> genl_unlock();
> err = __netlink_dump_start(net->genl_sock, skb, nlh, &c);
> genl_lock();
> -
> } else {
> struct netlink_dump_control c = {
> .module = family->module,
> - .data = info,
> - .start = ops->start,
> + .data = &ctx,
> + .start = genl_start,
> .dump = ops->dumpit,
> .done = genl_parallel_done,
> };
> --
> 2.26.2
>
