Re: [PATCH] tiny af_packet.c cleanup

Mitchell Blank Jr [off-list ref] :
[...]

I don't understand what you're saying - are you saying that the locking
isn't needed?  Or just the recheck isn't needed?  It sounds like if we're
only avoiding the race because of the bh_lock_sock() then we do need to
recheck, right?

- the locking is needed
- the recheck is needed
(- it does more than the recheck)

My point was related to the comment included in the patch, namely
"/* re-check under lock */"

From a reader's view, it is important to know why things are done and the

comment only tells _what_ is done.

So why is locking required ?

The comment just before the head of packet_rcv() suggests it.

As usual packet handler [1] in Linux, this code is run in a BH context.
It can race with user-space when it uses the struct sock related part of
the sk_buff (and it could race with tasklets when it messes with the
struct sk_buff itself)

User-space can change sk->sk_filter through setsockopt() +
SO_{ATTACH/DETACH}_FILTER but it takes care to only do so in sections 
surrounded by spin_{lock/unlock}_bh() [2]. As the first "if (sk->sk_filter)"
in packet_rcv() takes no lock, there is a time window where it is possible
that sk->sk_filter has been changed by user-space code on one cpu whereas
packet_rcv() still sees the old value. However, user is guaranteed that a
packet received _after_ his call to setsockopt() has returned will be
correctly handled by packet_rcv(). Whence 1) user is happy 2) packet_rcv()
isn't forced to take a lock before examination of sk->sk_filter when there
is no BPF handling to do (optimization).

Now let's assume some BPF-related action _appears_ to be needed. Without
locking, even filter = sk->sk_filter in packet_rcv() could race with
sk->sk_filter = NULL in sock_setsockopt() and result in partially set
content for "filter". That's why user-space code forbids BH on its CPU and
stops BH on others CPU with sk->sk_lock.slock. As soon as packet_rcv()
try to lock sk->sk_lock.slock, locking between packet_rcv() and user space
is fine.
As there is no point in issuing "forbid BH on my CPU" from packet_rcv(),
this part of the locking is forgotten. Thus packet_rcv() only issues a
spin_{lock/unlock} (which protects from tasklets on different CPU as well).
spin_{lock/unlock} is named bh_{lock/unlock}_sock. I assume it is on
documentation purpose and to allow an evil plan in the future.

[1] net/core/dev.c::dev_add_pack() and struct packet_type in
    include/linux/netdevice.h
[2] net/core/sock.c::sock_setsockopt() ... SO_DETACH_FILTER and
    net/core/filter.c::sk_attach_filter()

Could you do a patch for what you think it should look like?  You obviously
understand the locking issues here better than I.

See previously posted patch. Imho the non-trivial part isn't the locking
itself but the fact that the first test of sk->sk_filter is done _without_
lock. May be something like the following comment before this test:

/* Racing with user-space for optimization purpose: don't panic */

Tell me if it is correctly worded and I'll patch it.

Btw, the locking issues are rather well explained in 
Documentation/DocBook/kernel-locking.tmpl (and in Schimmel's book as well).

--
Ueimor