On 07/16/2020 07:47 PM, Konstantin Khorenko wrote:
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.
Nope, i've recalled my checks!
static const struct nfnetlink_subsystem nf_tables_subsys = {
.name = "nf_tables",
.subsys_id = NFNL_SUBSYS_NFTABLES,
.cb = nf_tables_cb,
...
static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
[NFT_MSG_NEWRULE] = {
.call_batch = nf_tables_newrule,
},
...
nfnetlink_find_client(u_int16_t type, const struct nfnetlink_subsystem *ss)
{
u_int8_t cb_id = NFNL_MSG_TYPE(type);
return &ss->cb[cb_id];
}
nfnetlink_rcv_batch(subsys_id)
{
...
nfnl_lock(subsys_id);
ss = rcu_dereference_protected(table[subsys_id].subsys, ...
...
/* We only accept a batch with messages for the same
* subsystem.
*/
if (NFNL_SUBSYS_ID(type) != subsys_id) {
err = -EINVAL;
goto ack;
}
nc = nfnetlink_find_client(type, ss);
...
nc->call_batch()
If nc->call_batch() == nf_tables_newrule(), it means
nc == nf_tables_cb[NFT_MSG_NEWRULE]
nc == &ss->cb[cb_id]
=> ss == nf_tables_subsys
and we know that nf_tables_subsys.subsys_id = NFNL_SUBSYS_NFTABLES
static const struct nfnetlink_subsystem nf_tables_subsys = {
.name = "nf_tables",
.subsys_id = NFNL_SUBSYS_NFTABLES,
.cb = nf_tables_cb,
...
Proven?
+ 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
