* [PATCH net-next] net: sched: action_ife: take reference to meta module
@ 2018-09-03 7:10 Vlad Buslov
2018-09-03 17:03 ` Cong Wang
2018-09-03 17:43 ` Cong Wang
0 siblings, 2 replies; 8+ messages in thread
From: Vlad Buslov @ 2018-09-03 7:10 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
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_cl
ass 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:ffff880354d37068 EFLAGS: 00010286
[ 646.470599] RAX: 0000000000000000 RBX: ffffffffc0a52518 RCX: ffffffff8c2668db
[ 646.478118] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: ffffffffc0a52518
[ 646.485641] RBP: ffffffffc0a52180 R08: fffffbfff814a4a4 R09: fffffbfff814a4a3
[ 646.493164] R10: ffffffffc0a5251b R11: fffffbfff814a4a4 R12: 1ffff1006a9a6e0d
[ 646.500687] R13: 00000000ffffffff R14: ffff880362bab890 R15: dead000000000100
[ 646.508213] FS: 00007f4164c99800(0000) GS:ffff88036fe40000(0000) knlGS:0000000000000000
[ 646.516961] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 646.523080] CR2: 00007f41638b8420 CR3: 0000000351df0004 CR4: 00000000001606e0
[ 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.622227] ? 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 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb cd 66 0f 1f 44 00 00 83 3d b9 d5 2b 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 be cd 00 00 48 89 04 24
[ 646.791474] RSP: 002b:00007ffdef7d6b58 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 646.799721] RAX: ffffffffffffffda RBX: 0000000000000024 RCX: 00007f4163872150
[ 646.807240] RDX: 0000000000000000 RSI: 00007ffdef7d6bd0 RDI: 0000000000000003
[ 646.814760] RBP: 000000005b8b9482 R08: 0000000000000001 R09: 0000000000000000
[ 646.822286] R10: 00000000000005e7 R11: 0000000000000246 R12: 00007ffdef7dad20
[ 646.829807] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000679bc0
[ 646.837360] irq event stamp: 6083
[ 646.841043] hardirqs last enabled at (6081): [<ffffffff8c220a7d>] __call_rcu+0x17d/0x500
[ 646.849882] hardirqs last disabled at (6083): [<ffffffff8c004f06>] trace_hardirqs_off_thunk+0x1a/0x1c
[ 646.859775] softirqs last enabled at (5968): [<ffffffff8d4004a1>] __do_softirq+0x4a1/0x6ee
[ 646.868784] softirqs last disabled at (6082): [<ffffffffc0a78759>] tcf_ife_cleanup+0x39/0x200 [act_ife]
[ 646.878845] ---[ end trace b1b8c12ffe51e657 ]---
Fixes: 5ffe57da29b3 ("act_ife: fix a potential deadlock")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_ife.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 19454146f60d..d8ea10259934 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -326,6 +326,21 @@ static int __add_metainfo(const struct tcf_meta_ops *ops,
return ret;
}
+static int add_metainfo_and_get_ops(const struct tcf_meta_ops *ops,
+ struct tcf_ife_info *ife, u32 metaid,
+ void *metaval, int len, bool atomic,
+ bool exists)
+{
+ int ret;
+
+ if (!try_module_get(ops->owner))
+ return -ENOENT;
+ ret = __add_metainfo(ops, ife, metaid, metaval, len, false, exists);
+ if (ret)
+ module_put(ops->owner);
+ return ret;
+}
+
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(&ife_mod_lock);
list_for_each_entry(o, &ifeoplist, 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;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
2018-09-03 7:10 [PATCH net-next] net: sched: action_ife: take reference to meta module Vlad Buslov
@ 2018-09-03 17:03 ` Cong Wang
2018-09-03 17:11 ` Vlad Buslov
2018-09-03 17:43 ` Cong Wang
1 sibling, 1 reply; 8+ messages in thread
From: Cong Wang @ 2018-09-03 17:03 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov <vladbu@mellanox.com> 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(&ife_mod_lock);
> list_for_each_entry(o, &ifeoplist, 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
2018-09-03 17:03 ` Cong Wang
@ 2018-09-03 17:11 ` Vlad Buslov
2018-09-03 17:36 ` Cong Wang
0 siblings, 1 reply; 8+ messages in thread
From: Vlad Buslov @ 2018-09-03 17:11 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
On Mon 03 Sep 2018 at 17:03, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov <vladbu@mellanox.com> 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(&ife_mod_lock);
>> list_for_each_entry(o, &ifeoplist, 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
2018-09-03 17:11 ` Vlad Buslov
@ 2018-09-03 17:36 ` Cong Wang
0 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2018-09-03 17:36 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
On Mon, Sep 3, 2018 at 10:12 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Mon 03 Sep 2018 at 17:03, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov <vladbu@mellanox.com> 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(&ife_mod_lock);
> >> list_for_each_entry(o, &ifeoplist, 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next] net: sched: action_ife: take reference to meta module
2018-09-03 7:10 [PATCH net-next] net: sched: action_ife: take reference to meta module Vlad Buslov
2018-09-03 17:03 ` Cong Wang
@ 2018-09-03 17:43 ` Cong Wang
2018-09-03 21:44 ` [PATCH net-next v2] " Vlad Buslov
1 sibling, 1 reply; 8+ messages in thread
From: Cong Wang @ 2018-09-03 17:43 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov <vladbu@mellanox.com> 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 <vladbu@mellanox.com>
This patch should be applied to -net rather than -net-next.
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
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(&ife_mod_lock);
> list_for_each_entry(o, &ifeoplist, 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v2] net: sched: action_ife: take reference to meta module
2018-09-03 17:43 ` Cong Wang
@ 2018-09-03 21:44 ` Vlad Buslov
2018-09-04 18:32 ` Cong Wang
2018-09-04 19:21 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Vlad Buslov @ 2018-09-03 21:44 UTC (permalink / raw)
To: netdev, xiyou.wangcong; +Cc: jhs, jiri, davem, Vlad Buslov
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_cl
ass 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:ffff880354d37068 EFLAGS: 00010286
[ 646.470599] RAX: 0000000000000000 RBX: ffffffffc0a52518 RCX: ffffffff8c2668db
[ 646.478118] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: ffffffffc0a52518
[ 646.485641] RBP: ffffffffc0a52180 R08: fffffbfff814a4a4 R09: fffffbfff814a4a3
[ 646.493164] R10: ffffffffc0a5251b R11: fffffbfff814a4a4 R12: 1ffff1006a9a6e0d
[ 646.500687] R13: 00000000ffffffff R14: ffff880362bab890 R15: dead000000000100
[ 646.508213] FS: 00007f4164c99800(0000) GS:ffff88036fe40000(0000) knlGS:0000000000000000
[ 646.516961] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 646.523080] CR2: 00007f41638b8420 CR3: 0000000351df0004 CR4: 00000000001606e0
[ 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.622227] ? 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 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb cd 66 0f 1f 44 00 00 83 3d b9 d5 2b 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 be cd 00 00 48 89 04 24
[ 646.791474] RSP: 002b:00007ffdef7d6b58 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 646.799721] RAX: ffffffffffffffda RBX: 0000000000000024 RCX: 00007f4163872150
[ 646.807240] RDX: 0000000000000000 RSI: 00007ffdef7d6bd0 RDI: 0000000000000003
[ 646.814760] RBP: 000000005b8b9482 R08: 0000000000000001 R09: 0000000000000000
[ 646.822286] R10: 00000000000005e7 R11: 0000000000000246 R12: 00007ffdef7dad20
[ 646.829807] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000679bc0
[ 646.837360] irq event stamp: 6083
[ 646.841043] hardirqs last enabled at (6081): [<ffffffff8c220a7d>] __call_rcu+0x17d/0x500
[ 646.849882] hardirqs last disabled at (6083): [<ffffffff8c004f06>] trace_hardirqs_off_thunk+0x1a/0x1c
[ 646.859775] softirqs last enabled at (5968): [<ffffffff8d4004a1>] __do_softirq+0x4a1/0x6ee
[ 646.868784] softirqs last disabled at (6082): [<ffffffffc0a78759>] tcf_ife_cleanup+0x39/0x200 [act_ife]
[ 646.878845] ---[ end trace b1b8c12ffe51e657 ]---
Fixes: 5ffe57da29b3 ("act_ife: fix a potential deadlock")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes V1->V2:
- fold constants into helper function
net/sched/act_ife.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 19454146f60d..ffd77e3fc2b6 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -326,6 +326,20 @@ static int __add_metainfo(const struct tcf_meta_ops *ops,
return ret;
}
+static int add_metainfo_and_get_ops(const struct tcf_meta_ops *ops,
+ struct tcf_ife_info *ife, u32 metaid,
+ bool exists)
+{
+ int ret;
+
+ if (!try_module_get(ops->owner))
+ return -ENOENT;
+ ret = __add_metainfo(ops, ife, metaid, NULL, 0, true, exists);
+ if (ret)
+ module_put(ops->owner);
+ return ret;
+}
+
static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
int len, bool exists)
{
@@ -349,7 +363,7 @@ static int use_all_metadata(struct tcf_ife_info *ife, bool exists)
read_lock(&ife_mod_lock);
list_for_each_entry(o, &ifeoplist, list) {
- rc = __add_metainfo(o, ife, o->metaid, NULL, 0, true, exists);
+ rc = add_metainfo_and_get_ops(o, ife, o->metaid, exists);
if (rc == 0)
installed += 1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] net: sched: action_ife: take reference to meta module
2018-09-03 21:44 ` [PATCH net-next v2] " Vlad Buslov
@ 2018-09-04 18:32 ` Cong Wang
2018-09-04 19:21 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: Cong Wang @ 2018-09-04 18:32 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
On Mon, Sep 3, 2018 at 2:44 PM Vlad Buslov <vladbu@mellanox.com> 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.
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
This one should go to -net too.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v2] net: sched: action_ife: take reference to meta module
2018-09-03 21:44 ` [PATCH net-next v2] " Vlad Buslov
2018-09-04 18:32 ` Cong Wang
@ 2018-09-04 19:21 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: David Miller @ 2018-09-04 19:21 UTC (permalink / raw)
To: vladbu; +Cc: netdev, xiyou.wangcong, jhs, jiri
From: Vlad Buslov <vladbu@mellanox.com>
Date: Tue, 4 Sep 2018 00:44:42 +0300
> 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:
...
> Fixes: 5ffe57da29b3 ("act_ife: fix a potential deadlock")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> ---
>
> Changes V1->V2:
> - fold constants into helper function
Applied to 'net'.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-09-04 23:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-03 7:10 [PATCH net-next] net: sched: action_ife: take reference to meta module Vlad Buslov
2018-09-03 17:03 ` Cong Wang
2018-09-03 17:11 ` Vlad Buslov
2018-09-03 17:36 ` Cong Wang
2018-09-03 17:43 ` Cong Wang
2018-09-03 21:44 ` [PATCH net-next v2] " Vlad Buslov
2018-09-04 18:32 ` Cong Wang
2018-09-04 19:21 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.