diff mbox series

audit: fix memory leak in nf_tables_commit

Message ID 20210713094158.450434-1-mudongliangabcd@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series audit: fix memory leak in nf_tables_commit | expand

Checks

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 10 of 10 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 fail Errors and warnings before: 280 this patch: 6
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 30 lines checked
netdev/build_allmodconfig_warn fail Errors and warnings before: 291 this patch: 6
netdev/header_inline success Link

Commit Message

Dongliang Mu July 13, 2021, 9:41 a.m. UTC
In nf_tables_commit, if nf_tables_commit_audit_alloc fails, it does not
free the adp variable.

Fix this by freeing the linked list with head adl.

backtrace:
  kmalloc include/linux/slab.h:591 [inline]
  kzalloc include/linux/slab.h:721 [inline]
  nf_tables_commit_audit_alloc net/netfilter/nf_tables_api.c:8439 [inline]
  nf_tables_commit+0x16e/0x1760 net/netfilter/nf_tables_api.c:8508
  nfnetlink_rcv_batch+0x512/0xa80 net/netfilter/nfnetlink.c:562
  nfnetlink_rcv_skb_batch net/netfilter/nfnetlink.c:634 [inline]
  nfnetlink_rcv+0x1fa/0x220 net/netfilter/nfnetlink.c:652
  netlink_unicast_kernel net/netlink/af_netlink.c:1314 [inline]
  netlink_unicast+0x2c7/0x3e0 net/netlink/af_netlink.c:1340
  netlink_sendmsg+0x36b/0x6b0 net/netlink/af_netlink.c:1929
  sock_sendmsg_nosec net/socket.c:702 [inline]
  sock_sendmsg+0x56/0x80 net/socket.c:722

Reported-by: syzbot <syzkaller@googlegroups.com>
Fixes: c520292f29b8 ("audit: log nftables configuration change events once per table")
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
 net/netfilter/nf_tables_api.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Lukas Bulwahn July 13, 2021, 11:47 a.m. UTC | #1
On Tue, Jul 13, 2021 at 11:42 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> In nf_tables_commit, if nf_tables_commit_audit_alloc fails, it does not
> free the adp variable.
>
> Fix this by freeing the linked list with head adl.
>
> backtrace:
>   kmalloc include/linux/slab.h:591 [inline]
>   kzalloc include/linux/slab.h:721 [inline]
>   nf_tables_commit_audit_alloc net/netfilter/nf_tables_api.c:8439 [inline]
>   nf_tables_commit+0x16e/0x1760 net/netfilter/nf_tables_api.c:8508
>   nfnetlink_rcv_batch+0x512/0xa80 net/netfilter/nfnetlink.c:562
>   nfnetlink_rcv_skb_batch net/netfilter/nfnetlink.c:634 [inline]
>   nfnetlink_rcv+0x1fa/0x220 net/netfilter/nfnetlink.c:652
>   netlink_unicast_kernel net/netlink/af_netlink.c:1314 [inline]
>   netlink_unicast+0x2c7/0x3e0 net/netlink/af_netlink.c:1340
>   netlink_sendmsg+0x36b/0x6b0 net/netlink/af_netlink.c:1929
>   sock_sendmsg_nosec net/socket.c:702 [inline]
>   sock_sendmsg+0x56/0x80 net/socket.c:722
>
> Reported-by: syzbot <syzkaller@googlegroups.com>

As far as I see, the more default way is to reference to syzbot by:

Reported-by: syzbot+[[20-letter hex reference]]@syzkaller.appspotmail.com

as in for example:

Reported-by: syzbot+fee64147a25aecd48055@syzkaller.appspotmail.com

A rough count says that format above is used 1300 times, whereas

Reported-by: syzbot <syzkaller@googlegroups.com>

is only used about 330 times.


Lukas

> Fixes: c520292f29b8 ("audit: log nftables configuration change events once per table")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  net/netfilter/nf_tables_api.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 390d4466567f..7f45b291be13 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -8444,6 +8444,16 @@ static int nf_tables_commit_audit_alloc(struct list_head *adl,
>         return 0;
>  }
>
> +static void nf_tables_commit_free(struct list_head *adl)
> +{
> +       struct nft_audit_data *adp, *adn;
> +
> +       list_for_each_entry_safe(adp, adn, adl, list) {
> +               list_del(&adp->list);
> +               kfree(adp);
> +       }
> +}
> +
>  static void nf_tables_commit_audit_collect(struct list_head *adl,
>                                            struct nft_table *table, u32 op)
>  {
> @@ -8508,6 +8518,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>                 ret = nf_tables_commit_audit_alloc(&adl, trans->ctx.table);
>                 if (ret) {
>                         nf_tables_commit_chain_prepare_cancel(net);
> +                       nf_tables_commit_free(adl);
>                         return ret;
>                 }
>                 if (trans->msg_type == NFT_MSG_NEWRULE ||
> @@ -8517,6 +8528,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>                         ret = nf_tables_commit_chain_prepare(net, chain);
>                         if (ret < 0) {
>                                 nf_tables_commit_chain_prepare_cancel(net);
> +                               nf_tables_commit_free(adl);
>                                 return ret;
>                         }
>                 }
> --
> 2.25.1
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20210713094158.450434-1-mudongliangabcd%40gmail.com.
Dongliang Mu July 13, 2021, 11:51 a.m. UTC | #2
On Tue, Jul 13, 2021 at 7:47 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 11:42 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> >
> > In nf_tables_commit, if nf_tables_commit_audit_alloc fails, it does not
> > free the adp variable.
> >
> > Fix this by freeing the linked list with head adl.
> >
> > backtrace:
> >   kmalloc include/linux/slab.h:591 [inline]
> >   kzalloc include/linux/slab.h:721 [inline]
> >   nf_tables_commit_audit_alloc net/netfilter/nf_tables_api.c:8439 [inline]
> >   nf_tables_commit+0x16e/0x1760 net/netfilter/nf_tables_api.c:8508
> >   nfnetlink_rcv_batch+0x512/0xa80 net/netfilter/nfnetlink.c:562
> >   nfnetlink_rcv_skb_batch net/netfilter/nfnetlink.c:634 [inline]
> >   nfnetlink_rcv+0x1fa/0x220 net/netfilter/nfnetlink.c:652
> >   netlink_unicast_kernel net/netlink/af_netlink.c:1314 [inline]
> >   netlink_unicast+0x2c7/0x3e0 net/netlink/af_netlink.c:1340
> >   netlink_sendmsg+0x36b/0x6b0 net/netlink/af_netlink.c:1929
> >   sock_sendmsg_nosec net/socket.c:702 [inline]
> >   sock_sendmsg+0x56/0x80 net/socket.c:722
> >
> > Reported-by: syzbot <syzkaller@googlegroups.com>
>
> As far as I see, the more default way is to reference to syzbot by:
>
> Reported-by: syzbot+[[20-letter hex reference]]@syzkaller.appspotmail.com
>

Hi Lukas,

this bug is not listed on the syzbot dashboard. I found this bug by
setting up a local syzkaller instance, so I only list syzbot other
than concrete syzbot id.

best regards,
Dongliang Mu

> as in for example:
>
> Reported-by: syzbot+fee64147a25aecd48055@syzkaller.appspotmail.com
>
> A rough count says that format above is used 1300 times, whereas
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
>
> is only used about 330 times.
>
>
> Lukas
>
> > Fixes: c520292f29b8 ("audit: log nftables configuration change events once per table")
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> >  net/netfilter/nf_tables_api.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 390d4466567f..7f45b291be13 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -8444,6 +8444,16 @@ static int nf_tables_commit_audit_alloc(struct list_head *adl,
> >         return 0;
> >  }
> >
> > +static void nf_tables_commit_free(struct list_head *adl)
> > +{
> > +       struct nft_audit_data *adp, *adn;
> > +
> > +       list_for_each_entry_safe(adp, adn, adl, list) {
> > +               list_del(&adp->list);
> > +               kfree(adp);
> > +       }
> > +}
> > +
> >  static void nf_tables_commit_audit_collect(struct list_head *adl,
> >                                            struct nft_table *table, u32 op)
> >  {
> > @@ -8508,6 +8518,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
> >                 ret = nf_tables_commit_audit_alloc(&adl, trans->ctx.table);
> >                 if (ret) {
> >                         nf_tables_commit_chain_prepare_cancel(net);
> > +                       nf_tables_commit_free(adl);
> >                         return ret;
> >                 }
> >                 if (trans->msg_type == NFT_MSG_NEWRULE ||
> > @@ -8517,6 +8528,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
> >                         ret = nf_tables_commit_chain_prepare(net, chain);
> >                         if (ret < 0) {
> >                                 nf_tables_commit_chain_prepare_cancel(net);
> > +                               nf_tables_commit_free(adl);
> >                                 return ret;
> >                         }
> >                 }
> > --
> > 2.25.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "syzkaller" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20210713094158.450434-1-mudongliangabcd%40gmail.com.
Lukas Bulwahn July 13, 2021, 12:04 p.m. UTC | #3
On Tue, Jul 13, 2021 at 1:52 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 7:47 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >
> > On Tue, Jul 13, 2021 at 11:42 AM Dongliang Mu <mudongliangabcd@gmail.com> wrote:
> > >

> > >
> > > Reported-by: syzbot <syzkaller@googlegroups.com>
> >
> > As far as I see, the more default way is to reference to syzbot by:
> >
> > Reported-by: syzbot+[[20-letter hex reference]]@syzkaller.appspotmail.com
> >
>
> Hi Lukas,
>
> this bug is not listed on the syzbot dashboard. I found this bug by
> setting up a local syzkaller instance, so I only list syzbot other
> than concrete syzbot id.
>

I see. Thanks for your explanation.

Lukas
kernel test robot July 13, 2021, 12:30 p.m. UTC | #4
Hi Dongliang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nf/master]
[also build test ERROR on nf-next/master ipvs/master v5.14-rc1 next-20210713]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dongliang-Mu/audit-fix-memory-leak-in-nf_tables_commit/20210713-174434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
config: alpha-randconfig-r002-20210713 (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/2112ee88ee1fa56b43d8d4ba2554d8d94199bd37
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dongliang-Mu/audit-fix-memory-leak-in-nf_tables_commit/20210713-174434
        git checkout 2112ee88ee1fa56b43d8d4ba2554d8d94199bd37
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash net/netfilter/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/netfilter/nf_tables_api.c: In function 'nf_tables_commit':
>> net/netfilter/nf_tables_api.c:8522:26: error: incompatible type for argument 1 of 'nf_tables_commit_free'
    8522 |    nf_tables_commit_free(adl);
         |                          ^~~
         |                          |
         |                          struct list_head
   net/netfilter/nf_tables_api.c:8448:53: note: expected 'struct list_head *' but argument is of type 'struct list_head'
    8448 | static void nf_tables_commit_free(struct list_head *adl)
         |                                   ~~~~~~~~~~~~~~~~~~^~~
   net/netfilter/nf_tables_api.c:8532:27: error: incompatible type for argument 1 of 'nf_tables_commit_free'
    8532 |     nf_tables_commit_free(adl);
         |                           ^~~
         |                           |
         |                           struct list_head
   net/netfilter/nf_tables_api.c:8448:53: note: expected 'struct list_head *' but argument is of type 'struct list_head'
    8448 | static void nf_tables_commit_free(struct list_head *adl)
         |                                   ~~~~~~~~~~~~~~~~~~^~~


vim +/nf_tables_commit_free +8522 net/netfilter/nf_tables_api.c

  8491	
  8492	static int nf_tables_commit(struct net *net, struct sk_buff *skb)
  8493	{
  8494		struct nftables_pernet *nft_net = nft_pernet(net);
  8495		struct nft_trans *trans, *next;
  8496		struct nft_trans_elem *te;
  8497		struct nft_chain *chain;
  8498		struct nft_table *table;
  8499		LIST_HEAD(adl);
  8500		int err;
  8501	
  8502		if (list_empty(&nft_net->commit_list)) {
  8503			mutex_unlock(&nft_net->commit_mutex);
  8504			return 0;
  8505		}
  8506	
  8507		/* 0. Validate ruleset, otherwise roll back for error reporting. */
  8508		if (nf_tables_validate(net) < 0)
  8509			return -EAGAIN;
  8510	
  8511		err = nft_flow_rule_offload_commit(net);
  8512		if (err < 0)
  8513			return err;
  8514	
  8515		/* 1.  Allocate space for next generation rules_gen_X[] */
  8516		list_for_each_entry_safe(trans, next, &nft_net->commit_list, list) {
  8517			int ret;
  8518	
  8519			ret = nf_tables_commit_audit_alloc(&adl, trans->ctx.table);
  8520			if (ret) {
  8521				nf_tables_commit_chain_prepare_cancel(net);
> 8522				nf_tables_commit_free(adl);
  8523				return ret;
  8524			}
  8525			if (trans->msg_type == NFT_MSG_NEWRULE ||
  8526			    trans->msg_type == NFT_MSG_DELRULE) {
  8527				chain = trans->ctx.chain;
  8528	
  8529				ret = nf_tables_commit_chain_prepare(net, chain);
  8530				if (ret < 0) {
  8531					nf_tables_commit_chain_prepare_cancel(net);
  8532					nf_tables_commit_free(adl);
  8533					return ret;
  8534				}
  8535			}
  8536		}
  8537	
  8538		/* step 2.  Make rules_gen_X visible to packet path */
  8539		list_for_each_entry(table, &nft_net->tables, list) {
  8540			list_for_each_entry(chain, &table->chains, list)
  8541				nf_tables_commit_chain(net, chain);
  8542		}
  8543	
  8544		/*
  8545		 * Bump generation counter, invalidate any dump in progress.
  8546		 * Cannot fail after this point.
  8547		 */
  8548		while (++nft_net->base_seq == 0)
  8549			;
  8550	
  8551		/* step 3. Start new generation, rules_gen_X now in use. */
  8552		net->nft.gencursor = nft_gencursor_next(net);
  8553	
  8554		list_for_each_entry_safe(trans, next, &nft_net->commit_list, list) {
  8555			nf_tables_commit_audit_collect(&adl, trans->ctx.table,
  8556						       trans->msg_type);
  8557			switch (trans->msg_type) {
  8558			case NFT_MSG_NEWTABLE:
  8559				if (nft_trans_table_update(trans)) {
  8560					if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
  8561						nft_trans_destroy(trans);
  8562						break;
  8563					}
  8564					if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
  8565						nf_tables_table_disable(net, trans->ctx.table);
  8566	
  8567					trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
  8568				} else {
  8569					nft_clear(net, trans->ctx.table);
  8570				}
  8571				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
  8572				nft_trans_destroy(trans);
  8573				break;
  8574			case NFT_MSG_DELTABLE:
  8575				list_del_rcu(&trans->ctx.table->list);
  8576				nf_tables_table_notify(&trans->ctx, NFT_MSG_DELTABLE);
  8577				break;
  8578			case NFT_MSG_NEWCHAIN:
  8579				if (nft_trans_chain_update(trans)) {
  8580					nft_chain_commit_update(trans);
  8581					nf_tables_chain_notify(&trans->ctx, NFT_MSG_NEWCHAIN);
  8582					/* trans destroyed after rcu grace period */
  8583				} else {
  8584					nft_chain_commit_drop_policy(trans);
  8585					nft_clear(net, trans->ctx.chain);
  8586					nf_tables_chain_notify(&trans->ctx, NFT_MSG_NEWCHAIN);
  8587					nft_trans_destroy(trans);
  8588				}
  8589				break;
  8590			case NFT_MSG_DELCHAIN:
  8591				nft_chain_del(trans->ctx.chain);
  8592				nf_tables_chain_notify(&trans->ctx, NFT_MSG_DELCHAIN);
  8593				nf_tables_unregister_hook(trans->ctx.net,
  8594							  trans->ctx.table,
  8595							  trans->ctx.chain);
  8596				break;
  8597			case NFT_MSG_NEWRULE:
  8598				nft_clear(trans->ctx.net, nft_trans_rule(trans));
  8599				nf_tables_rule_notify(&trans->ctx,
  8600						      nft_trans_rule(trans),
  8601						      NFT_MSG_NEWRULE);
  8602				nft_trans_destroy(trans);
  8603				break;
  8604			case NFT_MSG_DELRULE:
  8605				list_del_rcu(&nft_trans_rule(trans)->list);
  8606				nf_tables_rule_notify(&trans->ctx,
  8607						      nft_trans_rule(trans),
  8608						      NFT_MSG_DELRULE);
  8609				nft_rule_expr_deactivate(&trans->ctx,
  8610							 nft_trans_rule(trans),
  8611							 NFT_TRANS_COMMIT);
  8612				break;
  8613			case NFT_MSG_NEWSET:
  8614				nft_clear(net, nft_trans_set(trans));
  8615				/* This avoids hitting -EBUSY when deleting the table
  8616				 * from the transaction.
  8617				 */
  8618				if (nft_set_is_anonymous(nft_trans_set(trans)) &&
  8619				    !list_empty(&nft_trans_set(trans)->bindings))
  8620					trans->ctx.table->use--;
  8621	
  8622				nf_tables_set_notify(&trans->ctx, nft_trans_set(trans),
  8623						     NFT_MSG_NEWSET, GFP_KERNEL);
  8624				nft_trans_destroy(trans);
  8625				break;
  8626			case NFT_MSG_DELSET:
  8627				list_del_rcu(&nft_trans_set(trans)->list);
  8628				nf_tables_set_notify(&trans->ctx, nft_trans_set(trans),
  8629						     NFT_MSG_DELSET, GFP_KERNEL);
  8630				break;
  8631			case NFT_MSG_NEWSETELEM:
  8632				te = (struct nft_trans_elem *)trans->data;
  8633	
  8634				nft_setelem_activate(net, te->set, &te->elem);
  8635				nf_tables_setelem_notify(&trans->ctx, te->set,
  8636							 &te->elem,
  8637							 NFT_MSG_NEWSETELEM, 0);
  8638				nft_trans_destroy(trans);
  8639				break;
  8640			case NFT_MSG_DELSETELEM:
  8641				te = (struct nft_trans_elem *)trans->data;
  8642	
  8643				nf_tables_setelem_notify(&trans->ctx, te->set,
  8644							 &te->elem,
  8645							 NFT_MSG_DELSETELEM, 0);
  8646				nft_setelem_remove(net, te->set, &te->elem);
  8647				if (!nft_setelem_is_catchall(te->set, &te->elem)) {
  8648					atomic_dec(&te->set->nelems);
  8649					te->set->ndeact--;
  8650				}
  8651				break;
  8652			case NFT_MSG_NEWOBJ:
  8653				if (nft_trans_obj_update(trans)) {
  8654					nft_obj_commit_update(trans);
  8655					nf_tables_obj_notify(&trans->ctx,
  8656							     nft_trans_obj(trans),
  8657							     NFT_MSG_NEWOBJ);
  8658				} else {
  8659					nft_clear(net, nft_trans_obj(trans));
  8660					nf_tables_obj_notify(&trans->ctx,
  8661							     nft_trans_obj(trans),
  8662							     NFT_MSG_NEWOBJ);
  8663					nft_trans_destroy(trans);
  8664				}
  8665				break;
  8666			case NFT_MSG_DELOBJ:
  8667				nft_obj_del(nft_trans_obj(trans));
  8668				nf_tables_obj_notify(&trans->ctx, nft_trans_obj(trans),
  8669						     NFT_MSG_DELOBJ);
  8670				break;
  8671			case NFT_MSG_NEWFLOWTABLE:
  8672				if (nft_trans_flowtable_update(trans)) {
  8673					nft_trans_flowtable(trans)->data.flags =
  8674						nft_trans_flowtable_flags(trans);
  8675					nf_tables_flowtable_notify(&trans->ctx,
  8676								   nft_trans_flowtable(trans),
  8677								   &nft_trans_flowtable_hooks(trans),
  8678								   NFT_MSG_NEWFLOWTABLE);
  8679					list_splice(&nft_trans_flowtable_hooks(trans),
  8680						    &nft_trans_flowtable(trans)->hook_list);
  8681				} else {
  8682					nft_clear(net, nft_trans_flowtable(trans));
  8683					nf_tables_flowtable_notify(&trans->ctx,
  8684								   nft_trans_flowtable(trans),
  8685								   &nft_trans_flowtable(trans)->hook_list,
  8686								   NFT_MSG_NEWFLOWTABLE);
  8687				}
  8688				nft_trans_destroy(trans);
  8689				break;
  8690			case NFT_MSG_DELFLOWTABLE:
  8691				if (nft_trans_flowtable_update(trans)) {
  8692					nft_flowtable_hooks_del(nft_trans_flowtable(trans),
  8693								&nft_trans_flowtable_hooks(trans));
  8694					nf_tables_flowtable_notify(&trans->ctx,
  8695								   nft_trans_flowtable(trans),
  8696								   &nft_trans_flowtable_hooks(trans),
  8697								   NFT_MSG_DELFLOWTABLE);
  8698					nft_unregister_flowtable_net_hooks(net,
  8699									   &nft_trans_flowtable_hooks(trans));
  8700				} else {
  8701					list_del_rcu(&nft_trans_flowtable(trans)->list);
  8702					nf_tables_flowtable_notify(&trans->ctx,
  8703								   nft_trans_flowtable(trans),
  8704								   &nft_trans_flowtable(trans)->hook_list,
  8705								   NFT_MSG_DELFLOWTABLE);
  8706					nft_unregister_flowtable_net_hooks(net,
  8707							&nft_trans_flowtable(trans)->hook_list);
  8708				}
  8709				break;
  8710			}
  8711		}
  8712	
  8713		nft_commit_notify(net, NETLINK_CB(skb).portid);
  8714		nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
  8715		nf_tables_commit_audit_log(&adl, nft_net->base_seq);
  8716		nf_tables_commit_release(net);
  8717	
  8718		return 0;
  8719	}
  8720	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot July 13, 2021, 7:34 p.m. UTC | #5
Hi Dongliang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nf/master]
[also build test ERROR on nf-next/master ipvs/master v5.14-rc1 next-20210713]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dongliang-Mu/audit-fix-memory-leak-in-nf_tables_commit/20210713-174434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
config: x86_64-randconfig-a005-20210713 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8d69635ed9ecf36fd0ca85906bfde17949671cbe)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/2112ee88ee1fa56b43d8d4ba2554d8d94199bd37
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dongliang-Mu/audit-fix-memory-leak-in-nf_tables_commit/20210713-174434
        git checkout 2112ee88ee1fa56b43d8d4ba2554d8d94199bd37
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> net/netfilter/nf_tables_api.c:8522:26: error: passing 'struct list_head' to parameter of incompatible type 'struct list_head *'; take the address with &
                           nf_tables_commit_free(adl);
                                                 ^~~
                                                 &
   net/netfilter/nf_tables_api.c:8448:53: note: passing argument to parameter 'adl' here
   static void nf_tables_commit_free(struct list_head *adl)
                                                       ^
   net/netfilter/nf_tables_api.c:8532:27: error: passing 'struct list_head' to parameter of incompatible type 'struct list_head *'; take the address with &
                                   nf_tables_commit_free(adl);
                                                         ^~~
                                                         &
   net/netfilter/nf_tables_api.c:8448:53: note: passing argument to parameter 'adl' here
   static void nf_tables_commit_free(struct list_head *adl)
                                                       ^
   2 errors generated.


vim +8522 net/netfilter/nf_tables_api.c

  8491	
  8492	static int nf_tables_commit(struct net *net, struct sk_buff *skb)
  8493	{
  8494		struct nftables_pernet *nft_net = nft_pernet(net);
  8495		struct nft_trans *trans, *next;
  8496		struct nft_trans_elem *te;
  8497		struct nft_chain *chain;
  8498		struct nft_table *table;
  8499		LIST_HEAD(adl);
  8500		int err;
  8501	
  8502		if (list_empty(&nft_net->commit_list)) {
  8503			mutex_unlock(&nft_net->commit_mutex);
  8504			return 0;
  8505		}
  8506	
  8507		/* 0. Validate ruleset, otherwise roll back for error reporting. */
  8508		if (nf_tables_validate(net) < 0)
  8509			return -EAGAIN;
  8510	
  8511		err = nft_flow_rule_offload_commit(net);
  8512		if (err < 0)
  8513			return err;
  8514	
  8515		/* 1.  Allocate space for next generation rules_gen_X[] */
  8516		list_for_each_entry_safe(trans, next, &nft_net->commit_list, list) {
  8517			int ret;
  8518	
  8519			ret = nf_tables_commit_audit_alloc(&adl, trans->ctx.table);
  8520			if (ret) {
  8521				nf_tables_commit_chain_prepare_cancel(net);
> 8522				nf_tables_commit_free(adl);
  8523				return ret;
  8524			}
  8525			if (trans->msg_type == NFT_MSG_NEWRULE ||
  8526			    trans->msg_type == NFT_MSG_DELRULE) {
  8527				chain = trans->ctx.chain;
  8528	
  8529				ret = nf_tables_commit_chain_prepare(net, chain);
  8530				if (ret < 0) {
  8531					nf_tables_commit_chain_prepare_cancel(net);
  8532					nf_tables_commit_free(adl);
  8533					return ret;
  8534				}
  8535			}
  8536		}
  8537	
  8538		/* step 2.  Make rules_gen_X visible to packet path */
  8539		list_for_each_entry(table, &nft_net->tables, list) {
  8540			list_for_each_entry(chain, &table->chains, list)
  8541				nf_tables_commit_chain(net, chain);
  8542		}
  8543	
  8544		/*
  8545		 * Bump generation counter, invalidate any dump in progress.
  8546		 * Cannot fail after this point.
  8547		 */
  8548		while (++nft_net->base_seq == 0)
  8549			;
  8550	
  8551		/* step 3. Start new generation, rules_gen_X now in use. */
  8552		net->nft.gencursor = nft_gencursor_next(net);
  8553	
  8554		list_for_each_entry_safe(trans, next, &nft_net->commit_list, list) {
  8555			nf_tables_commit_audit_collect(&adl, trans->ctx.table,
  8556						       trans->msg_type);
  8557			switch (trans->msg_type) {
  8558			case NFT_MSG_NEWTABLE:
  8559				if (nft_trans_table_update(trans)) {
  8560					if (!(trans->ctx.table->flags & __NFT_TABLE_F_UPDATE)) {
  8561						nft_trans_destroy(trans);
  8562						break;
  8563					}
  8564					if (trans->ctx.table->flags & NFT_TABLE_F_DORMANT)
  8565						nf_tables_table_disable(net, trans->ctx.table);
  8566	
  8567					trans->ctx.table->flags &= ~__NFT_TABLE_F_UPDATE;
  8568				} else {
  8569					nft_clear(net, trans->ctx.table);
  8570				}
  8571				nf_tables_table_notify(&trans->ctx, NFT_MSG_NEWTABLE);
  8572				nft_trans_destroy(trans);
  8573				break;
  8574			case NFT_MSG_DELTABLE:
  8575				list_del_rcu(&trans->ctx.table->list);
  8576				nf_tables_table_notify(&trans->ctx, NFT_MSG_DELTABLE);
  8577				break;
  8578			case NFT_MSG_NEWCHAIN:
  8579				if (nft_trans_chain_update(trans)) {
  8580					nft_chain_commit_update(trans);
  8581					nf_tables_chain_notify(&trans->ctx, NFT_MSG_NEWCHAIN);
  8582					/* trans destroyed after rcu grace period */
  8583				} else {
  8584					nft_chain_commit_drop_policy(trans);
  8585					nft_clear(net, trans->ctx.chain);
  8586					nf_tables_chain_notify(&trans->ctx, NFT_MSG_NEWCHAIN);
  8587					nft_trans_destroy(trans);
  8588				}
  8589				break;
  8590			case NFT_MSG_DELCHAIN:
  8591				nft_chain_del(trans->ctx.chain);
  8592				nf_tables_chain_notify(&trans->ctx, NFT_MSG_DELCHAIN);
  8593				nf_tables_unregister_hook(trans->ctx.net,
  8594							  trans->ctx.table,
  8595							  trans->ctx.chain);
  8596				break;
  8597			case NFT_MSG_NEWRULE:
  8598				nft_clear(trans->ctx.net, nft_trans_rule(trans));
  8599				nf_tables_rule_notify(&trans->ctx,
  8600						      nft_trans_rule(trans),
  8601						      NFT_MSG_NEWRULE);
  8602				nft_trans_destroy(trans);
  8603				break;
  8604			case NFT_MSG_DELRULE:
  8605				list_del_rcu(&nft_trans_rule(trans)->list);
  8606				nf_tables_rule_notify(&trans->ctx,
  8607						      nft_trans_rule(trans),
  8608						      NFT_MSG_DELRULE);
  8609				nft_rule_expr_deactivate(&trans->ctx,
  8610							 nft_trans_rule(trans),
  8611							 NFT_TRANS_COMMIT);
  8612				break;
  8613			case NFT_MSG_NEWSET:
  8614				nft_clear(net, nft_trans_set(trans));
  8615				/* This avoids hitting -EBUSY when deleting the table
  8616				 * from the transaction.
  8617				 */
  8618				if (nft_set_is_anonymous(nft_trans_set(trans)) &&
  8619				    !list_empty(&nft_trans_set(trans)->bindings))
  8620					trans->ctx.table->use--;
  8621	
  8622				nf_tables_set_notify(&trans->ctx, nft_trans_set(trans),
  8623						     NFT_MSG_NEWSET, GFP_KERNEL);
  8624				nft_trans_destroy(trans);
  8625				break;
  8626			case NFT_MSG_DELSET:
  8627				list_del_rcu(&nft_trans_set(trans)->list);
  8628				nf_tables_set_notify(&trans->ctx, nft_trans_set(trans),
  8629						     NFT_MSG_DELSET, GFP_KERNEL);
  8630				break;
  8631			case NFT_MSG_NEWSETELEM:
  8632				te = (struct nft_trans_elem *)trans->data;
  8633	
  8634				nft_setelem_activate(net, te->set, &te->elem);
  8635				nf_tables_setelem_notify(&trans->ctx, te->set,
  8636							 &te->elem,
  8637							 NFT_MSG_NEWSETELEM, 0);
  8638				nft_trans_destroy(trans);
  8639				break;
  8640			case NFT_MSG_DELSETELEM:
  8641				te = (struct nft_trans_elem *)trans->data;
  8642	
  8643				nf_tables_setelem_notify(&trans->ctx, te->set,
  8644							 &te->elem,
  8645							 NFT_MSG_DELSETELEM, 0);
  8646				nft_setelem_remove(net, te->set, &te->elem);
  8647				if (!nft_setelem_is_catchall(te->set, &te->elem)) {
  8648					atomic_dec(&te->set->nelems);
  8649					te->set->ndeact--;
  8650				}
  8651				break;
  8652			case NFT_MSG_NEWOBJ:
  8653				if (nft_trans_obj_update(trans)) {
  8654					nft_obj_commit_update(trans);
  8655					nf_tables_obj_notify(&trans->ctx,
  8656							     nft_trans_obj(trans),
  8657							     NFT_MSG_NEWOBJ);
  8658				} else {
  8659					nft_clear(net, nft_trans_obj(trans));
  8660					nf_tables_obj_notify(&trans->ctx,
  8661							     nft_trans_obj(trans),
  8662							     NFT_MSG_NEWOBJ);
  8663					nft_trans_destroy(trans);
  8664				}
  8665				break;
  8666			case NFT_MSG_DELOBJ:
  8667				nft_obj_del(nft_trans_obj(trans));
  8668				nf_tables_obj_notify(&trans->ctx, nft_trans_obj(trans),
  8669						     NFT_MSG_DELOBJ);
  8670				break;
  8671			case NFT_MSG_NEWFLOWTABLE:
  8672				if (nft_trans_flowtable_update(trans)) {
  8673					nft_trans_flowtable(trans)->data.flags =
  8674						nft_trans_flowtable_flags(trans);
  8675					nf_tables_flowtable_notify(&trans->ctx,
  8676								   nft_trans_flowtable(trans),
  8677								   &nft_trans_flowtable_hooks(trans),
  8678								   NFT_MSG_NEWFLOWTABLE);
  8679					list_splice(&nft_trans_flowtable_hooks(trans),
  8680						    &nft_trans_flowtable(trans)->hook_list);
  8681				} else {
  8682					nft_clear(net, nft_trans_flowtable(trans));
  8683					nf_tables_flowtable_notify(&trans->ctx,
  8684								   nft_trans_flowtable(trans),
  8685								   &nft_trans_flowtable(trans)->hook_list,
  8686								   NFT_MSG_NEWFLOWTABLE);
  8687				}
  8688				nft_trans_destroy(trans);
  8689				break;
  8690			case NFT_MSG_DELFLOWTABLE:
  8691				if (nft_trans_flowtable_update(trans)) {
  8692					nft_flowtable_hooks_del(nft_trans_flowtable(trans),
  8693								&nft_trans_flowtable_hooks(trans));
  8694					nf_tables_flowtable_notify(&trans->ctx,
  8695								   nft_trans_flowtable(trans),
  8696								   &nft_trans_flowtable_hooks(trans),
  8697								   NFT_MSG_DELFLOWTABLE);
  8698					nft_unregister_flowtable_net_hooks(net,
  8699									   &nft_trans_flowtable_hooks(trans));
  8700				} else {
  8701					list_del_rcu(&nft_trans_flowtable(trans)->list);
  8702					nf_tables_flowtable_notify(&trans->ctx,
  8703								   nft_trans_flowtable(trans),
  8704								   &nft_trans_flowtable(trans)->hook_list,
  8705								   NFT_MSG_DELFLOWTABLE);
  8706					nft_unregister_flowtable_net_hooks(net,
  8707							&nft_trans_flowtable(trans)->hook_list);
  8708				}
  8709				break;
  8710			}
  8711		}
  8712	
  8713		nft_commit_notify(net, NETLINK_CB(skb).portid);
  8714		nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
  8715		nf_tables_commit_audit_log(&adl, nft_net->base_seq);
  8716		nf_tables_commit_release(net);
  8717	
  8718		return 0;
  8719	}
  8720	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 390d4466567f..7f45b291be13 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8444,6 +8444,16 @@  static int nf_tables_commit_audit_alloc(struct list_head *adl,
 	return 0;
 }
 
+static void nf_tables_commit_free(struct list_head *adl)
+{
+	struct nft_audit_data *adp, *adn;
+
+	list_for_each_entry_safe(adp, adn, adl, list) {
+		list_del(&adp->list);
+		kfree(adp);
+	}
+}
+
 static void nf_tables_commit_audit_collect(struct list_head *adl,
 					   struct nft_table *table, u32 op)
 {
@@ -8508,6 +8518,7 @@  static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 		ret = nf_tables_commit_audit_alloc(&adl, trans->ctx.table);
 		if (ret) {
 			nf_tables_commit_chain_prepare_cancel(net);
+			nf_tables_commit_free(adl);
 			return ret;
 		}
 		if (trans->msg_type == NFT_MSG_NEWRULE ||
@@ -8517,6 +8528,7 @@  static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 			ret = nf_tables_commit_chain_prepare(net, chain);
 			if (ret < 0) {
 				nf_tables_commit_chain_prepare_cancel(net);
+				nf_tables_commit_free(adl);
 				return ret;
 			}
 		}