On 6/21/24 00:12, Kuniyuki Iwashima wrote:
> Sorry for not mentioning this before, but could you replace "net" with
> "bpf" in Subject and rebase the patch on bpf.git so that we can trigger
> the patchwork's CI ?
No problem, will do.
>> ...
>> static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
>> {
>> + struct unix_sock *u = unix_sk(sk);
>> + struct sk_buff *skb;
>> + int err;
>> +
>> if (unlikely(READ_ONCE(sk->sk_state) != TCP_ESTABLISHED))
>> return -ENOTCONN;
>>
>> - return unix_read_skb(sk, recv_actor);
>> + mutex_lock(&u->iolock);
>> + skb = skb_recv_datagram(sk, MSG_DONTWAIT, &err);
>
> mutex_unlock(&u->iolock);
>
> I think we can drop mutex here as the skb is already unlinked
> and no receiver can touch it.
I guess you're right about the mutex. That said, double mea culpa, lack of
state lock makes things racy:
unix_stream_read_skb
mutex_lock
skb = skb_recv_datagram
mutex_unlock
spin_lock
if (oob_skb == skb) {
unix_release_sock
if (u->oob_skb) {
kfree_skb(u->oob_skb)
u->oob_skb = NULL
}
oob_skb = NULL
drop = true
}
spin_unlock
if (drop) {
skb_unref(skb)
kfree_skb(skb)
}
In v2 I'll do what unix_stream_read_generic() does: take state lock and
check for SOCK_DEAD.
> and the below part can be like the following not to slow down
> the common case:
>
> if (!skb)
> return err;
>
>> +
>> +#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>> + if (skb) {
>
> if (unlikely(skb == READ_ONCE(u->oob_skb))) {
>
>
>> + bool drop = false;
>> +
>> + spin_lock(&sk->sk_receive_queue.lock);
>> + if (skb == u->oob_skb) {
>
> if (likely(skb == u->oob_skb)) {
>
>> + WRITE_ONCE(u->oob_skb, NULL);
>> + drop = true;
>> + }
>> + spin_unlock(&sk->sk_receive_queue.lock);
>> +
>> + if (drop) {
>> + WARN_ON_ONCE(skb_unref(skb));
>> + kfree_skb(skb);
>> + skb = NULL;
>> + err = -EAGAIN;
> return -EAGAIN;
>
>> + }
>> + }
>> +#endif
>
> return recv_actor(sk, skb);
All right, thanks. So here's v2:
https://lore.kernel.org/netdev/20240622223324.3337956-1-mhal@xxxxxxx/
