Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov wrote:
>
> Recent refactoring of add_metainfo() caused use_all_metadata() to add
> metainfo to ife action metalist without taking reference to module. This
> causes warning in module_put called from ife action cleanup function.
>
> Implement add_metainfo_and_get_ops() function that returns with reference
> to module taken if metainfo was added successfully, and call it from
> use_all_metadata(), instead of calling __add_metainfo() directly.
>
...
>
> Fixes: 5ffe57da29b3 ("act_ife: fix a potential deadlock")
> Signed-off-by: Vlad Buslov
This patch should be applied to -net rather than -net-next.
Acked-by: Cong Wang
One nit below.
> static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
> int len, bool exists)
> {
> @@ -349,7 +364,8 @@ static int use_all_metadata(struct tcf_ife_info *ife,
> bool exists)
>
> read_lock(_mod_lock);
> list_for_each_entry(o, , list) {
> - rc = __add_metainfo(o, ife, o->metaid, NULL, 0, true, exists);
> + rc = add_metainfo_and_get_ops(o, ife, o->metaid, NULL, 0,
> true,
> + exists);
Nit: you can fold constants into this helper as it only has one caller.
Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
On Mon, Sep 3, 2018 at 10:12 AM Vlad Buslov wrote:
>
>
> On Mon 03 Sep 2018 at 17:03, Cong Wang wrote:
> > On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov wrote:
> >>
> >> Recent refactoring of add_metainfo() caused use_all_metadata() to add
> >> metainfo to ife action metalist without taking reference to module. This
> >> causes warning in module_put called from ife action cleanup function.
> >>
> >> Implement add_metainfo_and_get_ops() function that returns with reference
> >> to module taken if metainfo was added successfully, and call it from
> >> use_all_metadata(), instead of calling __add_metainfo() directly.
> >
> > Good catch!
> >
> > I thought every entry in ifeoplist must hold a refcnt to its module, looks
> > like I was wrong.
> >
> >
> >> read_lock(_mod_lock);
> >> list_for_each_entry(o, , list) {
> >> - rc = __add_metainfo(o, ife, o->metaid, NULL, 0, true,
> >> exists);
> >> + rc = add_metainfo_and_get_ops(o, ife, o->metaid, NULL, 0,
> >> true,
> >> + exists);
> >> if (rc == 0)
> >> installed += 1;
> >
> > I am afraid you have to rollback on failure inside this loop, that is,
> > releasing all previous module refcnt properly on error.
>
> Do I? This function looks like it is explicitly designed to succeed if
> at least one metainfo was successfully added. And this is how it was
> originally implemented before deadlock fix refactoring.
Hmm, well, it is taken care by tcf_idr_release() via _tcf_ife_cleanup()...
So, your patch should be fine.
Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
On Mon 03 Sep 2018 at 17:03, Cong Wang wrote:
> On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov wrote:
>>
>> Recent refactoring of add_metainfo() caused use_all_metadata() to add
>> metainfo to ife action metalist without taking reference to module. This
>> causes warning in module_put called from ife action cleanup function.
>>
>> Implement add_metainfo_and_get_ops() function that returns with reference
>> to module taken if metainfo was added successfully, and call it from
>> use_all_metadata(), instead of calling __add_metainfo() directly.
>
> Good catch!
>
> I thought every entry in ifeoplist must hold a refcnt to its module, looks
> like I was wrong.
>
>
>> read_lock(_mod_lock);
>> list_for_each_entry(o, , list) {
>> - rc = __add_metainfo(o, ife, o->metaid, NULL, 0, true,
>> exists);
>> + rc = add_metainfo_and_get_ops(o, ife, o->metaid, NULL, 0,
>> true,
>> + exists);
>> if (rc == 0)
>> installed += 1;
>
> I am afraid you have to rollback on failure inside this loop, that is,
> releasing all previous module refcnt properly on error.
Do I? This function looks like it is explicitly designed to succeed if
at least one metainfo was successfully added. And this is how it was
originally implemented before deadlock fix refactoring.
Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov wrote:
>
> Recent refactoring of add_metainfo() caused use_all_metadata() to add
> metainfo to ife action metalist without taking reference to module. This
> causes warning in module_put called from ife action cleanup function.
>
> Implement add_metainfo_and_get_ops() function that returns with reference
> to module taken if metainfo was added successfully, and call it from
> use_all_metadata(), instead of calling __add_metainfo() directly.
Good catch!
I thought every entry in ifeoplist must hold a refcnt to its module, looks
like I was wrong.
> read_lock(_mod_lock);
> list_for_each_entry(o, , list) {
> - rc = __add_metainfo(o, ife, o->metaid, NULL, 0, true, exists);
> + rc = add_metainfo_and_get_ops(o, ife, o->metaid, NULL, 0,
> true,
> + exists);
> if (rc == 0)
> installed += 1;
I am afraid you have to rollback on failure inside this loop, that is,
releasing all previous module refcnt properly on error.
[PATCH net-next] net: sched: action_ife: take reference to meta module
Recent refactoring of add_metainfo() caused use_all_metadata() to add metainfo to ife action metalist without taking reference to module. This causes warning in module_put called from ife action cleanup function. Implement add_metainfo_and_get_ops() function that returns with reference to module taken if metainfo was added successfully, and call it from use_all_metadata(), instead of calling __add_metainfo() directly. Example warning: [ 646.344393] WARNING: CPU: 1 PID: 2278 at kernel/module.c:1139 module_put+0x1cb/0x230 [ 646.352437] Modules linked in: act_meta_skbtcindex act_meta_mark act_meta_skbprio act_ife ife veth nfsv3 nfs fscache xt_CHECKSUM iptable_mangle ipt_MASQUERADE iptable_nat nf_nat_ipv4 nf_nat xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c tun ebtable_filter ebtables ip6table_filter ip6_tables bridge stp llc mlx5_ib ib_uverbs ib_core intel_rapl sb_edac x86_pkg_temp_thermal mlx5_core coretemp kvm_intel kvm nfsd igb irqbypass crct10dif_pclmul devlink crc32_pclmul mei_me joydev ses crc32c_intel enclosure auth_rpcgss i2c_algo_bit ioatdma ptp mei pps_core ghash_clmulni_intel iTCO_wdt iTCO_vendor_support pcspkr dca ipmi_ssif lpc_ich target_core_mod i2c_i801 ipmi_si ipmi_devintf pcc_cpufreq wmi ipmi_msghandler nfs_acl lockd acpi_pad acpi_power_meter grace sunrpc mpt3sas raid_class scsi_transport_sas [ 646.425631] CPU: 1 PID: 2278 Comm: tc Not tainted 4.19.0-rc1+ #799 [ 646.432187] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017 [ 646.440595] RIP: 0010:module_put+0x1cb/0x230 [ 646.445238] Code: f3 66 94 02 e8 26 ff fa ff 85 c0 74 11 0f b6 1d 51 30 94 02 80 fb 01 77 60 83 e3 01 74 13 65 ff 0d 3a 83 db 73 e9 2b ff ff ff <0f> 0b e9 00 ff ff ff e8 59 01 fb ff 85 c0 75 e4 48 c7 c2 20 62 6b [ 646.464997] RSP: 0018:880354d37068 EFLAGS: 00010286 [ 646.470599] RAX: RBX: c0a52518 RCX: 8c2668db [ 646.478118] RDX: 0003 RSI: dc00 RDI: c0a52518 [ 646.485641] RBP: c0a52180 R08: fbfff814a4a4 R09: fbfff814a4a3 [ 646.493164] R10: c0a5251b R11: fbfff814a4a4 R12: 11006a9a6e0d [ 646.500687] R13: R14: 880362bab890 R15: dead0100 [ 646.508213] FS: 7f4164c99800() GS:88036fe4() knlGS: [ 646.516961] CS: 0010 DS: ES: CR0: 80050033 [ 646.523080] CR2: 7f41638b8420 CR3: 000351df0004 CR4: 001606e0 [ 646.530595] Call Trace: [ 646.533408] ? find_symbol_in_section+0x260/0x260 [ 646.538509] tcf_ife_cleanup+0x11b/0x200 [act_ife] [ 646.543695] tcf_action_cleanup+0x29/0xa0 [ 646.548078] __tcf_action_put+0x5a/0xb0 [ 646.552289] ? nla_put+0x65/0xe0 [ 646.555889] __tcf_idr_release+0x48/0x60 [ 646.560187] tcf_generic_walker+0x448/0x6b0 [ 646.564764] ? tcf_action_dump_1+0x450/0x450 [ 646.569411] ? __lock_is_held+0x84/0x110 [ 646.573720] ? tcf_ife_walker+0x10c/0x20f [act_ife] [ 646.578982] tca_action_gd+0x972/0xc40 [ 646.583129] ? tca_get_fill.constprop.17+0x250/0x250 [ 646.588471] ? mark_lock+0xcf/0x980 [ 646.592324] ? check_chain_key+0x140/0x1f0 [ 646.596832] ? debug_show_all_locks+0x240/0x240 [ 646.601839] ? memset+0x1f/0x40 [ 646.605350] ? nla_parse+0xca/0x1a0 [ 646.609217] tc_ctl_action+0x215/0x230 [ 646.613339] ? tcf_action_add+0x220/0x220 [ 646.617748] rtnetlink_rcv_msg+0x56a/0x6d0 [ 646.67] ? rtnl_fdb_del+0x3f0/0x3f0 [ 646.626466] netlink_rcv_skb+0x18d/0x200 [ 646.630752] ? rtnl_fdb_del+0x3f0/0x3f0 [ 646.634959] ? netlink_ack+0x500/0x500 [ 646.639106] netlink_unicast+0x2d0/0x370 [ 646.643409] ? netlink_attachskb+0x340/0x340 [ 646.648050] ? _copy_from_iter_full+0xe9/0x3e0 [ 646.652870] ? import_iovec+0x11e/0x1c0 [ 646.657083] netlink_sendmsg+0x3b9/0x6a0 [ 646.661388] ? netlink_unicast+0x370/0x370 [ 646.665877] ? netlink_unicast+0x370/0x370 [ 646.670351] sock_sendmsg+0x6b/0x80 [ 646.674212] ___sys_sendmsg+0x4a1/0x520 [ 646.678443] ? copy_msghdr_from_user+0x210/0x210 [ 646.683463] ? lock_downgrade+0x320/0x320 [ 646.687849] ? debug_show_all_locks+0x240/0x240 [ 646.692760] ? do_raw_spin_unlock+0xa2/0x130 [ 646.697418] ? _raw_spin_unlock+0x24/0x30 [ 646.701798] ? __handle_mm_fault+0x1819/0x1c10 [ 646.706619] ? __pmd_alloc+0x320/0x320 [ 646.710738] ? debug_show_all_locks+0x240/0x240 [ 646.715649] ? restore_nameidata+0x7b/0xa0 [ 646.720117] ? check_chain_key+0x140/0x1f0 [ 646.724590] ? check_chain_key+0x140/0x1f0 [ 646.729070] ? __fget_light+0xbc/0xd0 [ 646.733121] ? __sys_sendmsg+0xd7/0x150 [ 646.737329] __sys_sendmsg+0xd7/0x150 [ 646.741359] ? __ia32_sys_shutdown+0x30/0x30 [ 646.746003] ? up_read+0x53/0x90 [ 646.749601] ? __do_page_fault+0x484/0x780 [ 646.754105] ? do_syscall_64+0x1e/0x2c0 [ 646.758320] do_syscall_64+0x72/0x2c0 [ 646.762353] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 646.767776] RIP: 0033:0x7f4163872150 [ 646.771713] Code: 8b 15 3c 7d 2b
