On Fri, Oct 30, 2015 at 4:00 AM, Eric Dumazet [off-list ref] wrote:

On Fri, 2015-10-30 at 11:48 +0100, Florian Westphal wrote:

quoted

Hannes Frederic Sowa [off-list ref] wrote:

quoted

quoted

quoted

quoted

@@ -936,7 +936,9 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt,

                      rtnl_unicast(skb, net, NETLINK_CB(skb).portid);
              } else {
+                     preempt_disable();
                      ip_mr_forward(net, mrt, skb, c, 0);
+                     preempt_enable();
              }
      }
 }

I do not believe this fix is correct.

Yes, sorry.  I should have suggested local_bh_disable instead.

quoted

Better replace the
IP_INC_STATS_BH() by IP_INC_STATS()

and IP_ADD_STATS_BH() by IP_ADD_STATS()

Hmm, whats the rationale for this?

Note that IP_ADD_STATS_BH in question is unconditional (not in
error path).  It seems that its virtually always called from softirq
except in the setsockopt case.

The naming of the functions is bad if you compare them to e.g.
spin_lock_bh.

STATS_BH can only be used from bottom half and the normal ones (without
_BH) can be called from everywhere. It is a common pattern in the
kernel.

Eric's proposal is correct.

Yes, its correct but it results in 4 additonal bh on/off calls
for the common case, hence my question.

Moving the one ip_mr_forward into bh-off keeps the bh-disable thing
in the setsockopt path.

I have no idea how long is the ip_mr_forward(net, mrt, skb, c, 0)
section, and if GFP_KERNEL allocations were attempted in this path.

The proposed fix might add other regressions.

I do not want to spend time auditing this code that nobody uses.

While on x86, IP_INC_STATS() does not use additional bh on/off calls

for 32 bit archs, it does in SNMP_ADD_STATS64_USER()

In general, we should disable interrupts (even if soft) for limited
amount of times.