On 07/16/2020 04:55 PM, Andrey Ryabinin wrote:
On 7/16/20 3:37 PM, Konstantin Khorenko wrote:
+extern struct nf_logger __rcu *loggers[NFPROTO_NUMPROTO][NF_LOG_TYPE_MAX]
__read_mostly;
+
+/*
+ * In "nft_log" module we need to drop nfnl lock while performing
+ * request_module(), calls to nf_logger_find_get() in other
+ * modules are done without nfnl_lock taken.
+ *
+ * nf_logger_find_get
+ * nft_log_init
+ * nf_tables_newexpr
+ * nf_tables_newrule // nc->call_batch
+ * // called with nfnl_lock taken
+ *
+ * nfnetlink_rcv_batch // takes nfnl_lock(NFNL_SUBSYS_NFTABLES)
+ * nfnetlink_rcv
+ * netlink_unicast
+ * netlink_sendmsg
+ */
+static int nf_logger_find_get_lock(int pf, enum nf_log_type type)
+{
+ struct nf_logger *logger;
+ int ret = 0;
+
+ logger = loggers[pf][type];
+ if (logger == NULL) {
+ nfnl_unlock(NFNL_SUBSYS_NFTABLES);
nfnetlink_rcv_batch() takes nfnl_lock(subsys_id) lock
How you can be sure that subsys_id is always NFNL_SUBSYS_NFTABLES ?
1) nf_logger_find_get_lock() is called in only 1 place - in nft_log_init()
2) nft_log_init() == nft_log_ops.init() == struct nft_expr_ops::init()
struct nft_expr_ops::init() is called only in nf_tables_newexpr()
=> nft_log_init() is called only in nf_tables_newexpr()
3) nf_tables_newexpr() is called only in nf_tables_newrule() and in
nft_expr_init()
a) nft_expr_init() is called only in nft_dynset_init()
nft_dynset_init() == nft_dynset_ops.init() == struct nft_expr_ops::init()
struct nft_expr_ops::init() is called only in nf_tables_newexpr()
=> nft_dynset_init() is called only in nf_tables_newexpr()
=> goto 3)
b) nf_tables_newrule() is the nf_tables_cb[NFT_MSG_NEWRULE].call_batch()
struct nfnl_callback nf_tables_cb[NFT_MSG_MAX];
struct nfnl_callback::call_batch() is called only in nfnetlink_rcv_batch()
(nc->call_batch())
=>
b.1) nf_tables_newrule() is called only in nfnetlink_rcv_batch()
b.2) nfnetlink_rcv_batch() acquires nfnl_lock(NFNL_SUBSYS_NFTABLES ???)
===========
/* Work around old nft using host byte order */
if (nfgenmsg->res_id == NFNL_SUBSYS_NFTABLES)
res_id = NFNL_SUBSYS_NFTABLES;
else
res_id = ntohs(nfgenmsg->res_id);
nfnetlink_rcv_batch(skb, nlh, res_id);
===========
Well, looks like i was not patient enough,
i've proven that nfnl_lock() is always taken by above
(i mean there is no path nf_logger_find_get() is called without nfnl_lock() is
taken),
but you are right, it can be not nfnl_lock(NFNL_SUBSYS_NFTABLES) but for some
other subsys_id.
Thank you for catching this, i will rework the patch.
+ request_module("nf-logger-%u-%u", pf, type);
+ nfnl_lock(NFNL_SUBSYS_NFTABLES);
+ ret = -EAGAIN;
+ }
.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
