Skip to content

Commit d45ed4a

Browse files
Alexei Starovoitovdavem330
Alexei Starovoitov
authored and
davem330
committed
net: fix unsafe set_memory_rw from softirq
on x86 system with net.core.bpf_jit_enable = 1 sudo tcpdump -i eth1 'tcp port 22' causes the warning: [ 56.766097] Possible unsafe locking scenario: [ 56.766097] [ 56.780146] CPU0 [ 56.786807] ---- [ 56.793188] lock(&(&vb->lock)->rlock); [ 56.799593] <Interrupt> [ 56.805889] lock(&(&vb->lock)->rlock); [ 56.812266] [ 56.812266] *** DEADLOCK *** [ 56.812266] [ 56.830670] 1 lock held by ksoftirqd/1/13: [ 56.836838] #0: (rcu_read_lock){.+.+..}, at: [<ffffffff8118f44c>] vm_unmap_aliases+0x8c/0x380 [ 56.849757] [ 56.849757] stack backtrace: [ 56.862194] CPU: 1 PID: 13 Comm: ksoftirqd/1 Not tainted 3.12.0-rc3+ #45 [ 56.868721] Hardware name: System manufacturer System Product Name/P8Z77 WS, BIOS 3007 07/26/2012 [ 56.882004] ffffffff821944c0 ffff88080bbdb8c8 ffffffff8175a145 0000000000000007 [ 56.895630] ffff88080bbd5f40 ffff88080bbdb928 ffffffff81755b14 0000000000000001 [ 56.909313] ffff880800000001 ffff880800000000 ffffffff8101178f 0000000000000001 [ 56.923006] Call Trace: [ 56.929532] [<ffffffff8175a145>] dump_stack+0x55/0x76 [ 56.936067] [<ffffffff81755b14>] print_usage_bug+0x1f7/0x208 [ 56.942445] [<ffffffff8101178f>] ? save_stack_trace+0x2f/0x50 [ 56.948932] [<ffffffff810cc0a0>] ? check_usage_backwards+0x150/0x150 [ 56.955470] [<ffffffff810ccb52>] mark_lock+0x282/0x2c0 [ 56.961945] [<ffffffff810ccfed>] __lock_acquire+0x45d/0x1d50 [ 56.968474] [<ffffffff810cce6e>] ? __lock_acquire+0x2de/0x1d50 [ 56.975140] [<ffffffff81393bf5>] ? cpumask_next_and+0x55/0x90 [ 56.981942] [<ffffffff810cef72>] lock_acquire+0x92/0x1d0 [ 56.988745] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380 [ 56.995619] [<ffffffff817628f1>] _raw_spin_lock+0x41/0x50 [ 57.002493] [<ffffffff8118f52a>] ? vm_unmap_aliases+0x16a/0x380 [ 57.009447] [<ffffffff8118f52a>] vm_unmap_aliases+0x16a/0x380 [ 57.016477] [<ffffffff8118f44c>] ? vm_unmap_aliases+0x8c/0x380 [ 57.023607] [<ffffffff810436b0>] change_page_attr_set_clr+0xc0/0x460 [ 57.030818] [<ffffffff810cfb8d>] ? trace_hardirqs_on+0xd/0x10 [ 57.037896] [<ffffffff811a8330>] ? kmem_cache_free+0xb0/0x2b0 [ 57.044789] [<ffffffff811b59c3>] ? free_object_rcu+0x93/0xa0 [ 57.051720] [<ffffffff81043d9f>] set_memory_rw+0x2f/0x40 [ 57.058727] [<ffffffff8104e17c>] bpf_jit_free+0x2c/0x40 [ 57.065577] [<ffffffff81642cba>] sk_filter_release_rcu+0x1a/0x30 [ 57.072338] [<ffffffff811108e2>] rcu_process_callbacks+0x202/0x7c0 [ 57.078962] [<ffffffff81057f17>] __do_softirq+0xf7/0x3f0 [ 57.085373] [<ffffffff81058245>] run_ksoftirqd+0x35/0x70 cannot reuse jited filter memory, since it's readonly, so use original bpf insns memory to hold work_struct defer kfree of sk_filter until jit completed freeing tested on x86_64 and i386 Signed-off-by: Alexei Starovoitov <[email protected]> Acked-by: Eric Dumazet <[email protected]> Signed-off-by: David S. Miller <[email protected]>
1 parent 582442d commit d45ed4a

File tree

8 files changed

+36
-18
lines changed

8 files changed

+36
-18
lines changed

‎arch/arm/net/bpf_jit_32.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -930,4 +930,5 @@ void bpf_jit_free(struct sk_filter *fp)
930930
{
931931
if (fp->bpf_func != sk_run_filter)
932932
module_free(NULL, fp->bpf_func);
933+
kfree(fp);
933934
}

‎arch/powerpc/net/bpf_jit_comp.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -691,4 +691,5 @@ void bpf_jit_free(struct sk_filter *fp)
691691
{
692692
if (fp->bpf_func != sk_run_filter)
693693
module_free(NULL, fp->bpf_func);
694+
kfree(fp);
694695
}

‎arch/s390/net/bpf_jit_comp.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -881,7 +881,9 @@ void bpf_jit_free(struct sk_filter *fp)
881881
struct bpf_binary_header *header = (void *)addr;
882882

883883
if (fp->bpf_func == sk_run_filter)
884-
return;
884+
goto free_filter;
885885
set_memory_rw(addr, header->pages);
886886
module_free(NULL, header);
887+
free_filter:
888+
kfree(fp);
887889
}

‎arch/sparc/net/bpf_jit_comp.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -808,4 +808,5 @@ void bpf_jit_free(struct sk_filter *fp)
808808
{
809809
if (fp->bpf_func != sk_run_filter)
810810
module_free(NULL, fp->bpf_func);
811+
kfree(fp);
811812
}

‎arch/x86/net/bpf_jit_comp.c

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -772,13 +772,21 @@ cond_branch: f_offset = addrs[i + filter[i].jf] - addrs[i];
772772
return;
773773
}
774774

775+
static void bpf_jit_free_deferred(struct work_struct *work)
776+
{
777+
struct sk_filter *fp = container_of(work, struct sk_filter, work);
778+
unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
779+
struct bpf_binary_header *header = (void *)addr;
780+
781+
set_memory_rw(addr, header->pages);
782+
module_free(NULL, header);
783+
kfree(fp);
784+
}
785+
775786
void bpf_jit_free(struct sk_filter *fp)
776787
{
777788
if (fp->bpf_func != sk_run_filter) {
778-
unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
779-
struct bpf_binary_header *header = (void *)addr;
780-
781-
set_memory_rw(addr, header->pages);
782-
module_free(NULL, header);
789+
INIT_WORK(&fp->work, bpf_jit_free_deferred);
790+
schedule_work(&fp->work);
783791
}
784792
}

‎include/linux/filter.h

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
#include <linux/atomic.h>
88
#include <linux/compat.h>
9+
#include <linux/workqueue.h>
910
#include <uapi/linux/filter.h>
1011

1112
#ifdef CONFIG_COMPAT
@@ -25,15 +26,19 @@ struct sk_filter
2526
{
2627
atomic_t refcnt;
2728
unsigned int len; /* Number of filter blocks */
29+
struct rcu_head rcu;
2830
unsigned int (*bpf_func)(const struct sk_buff *skb,
2931
const struct sock_filter *filter);
30-
struct rcu_head rcu;
31-
struct sock_filter insns[0];
32+
union {
33+
struct sock_filter insns[0];
34+
struct work_struct work;
35+
};
3236
};
3337

34-
static inline unsigned int sk_filter_len(const struct sk_filter *fp)
38+
static inline unsigned int sk_filter_size(unsigned int proglen)
3539
{
36-
return fp->len * sizeof(struct sock_filter) + sizeof(*fp);
40+
return max(sizeof(struct sk_filter),
41+
offsetof(struct sk_filter, insns[proglen]));
3742
}
3843

3944
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
@@ -67,11 +72,13 @@ static inline void bpf_jit_dump(unsigned int flen, unsigned int proglen,
6772
}
6873
#define SK_RUN_FILTER(FILTER, SKB) (*FILTER->bpf_func)(SKB, FILTER->insns)
6974
#else
75+
#include <linux/slab.h>
7076
static inline void bpf_jit_compile(struct sk_filter *fp)
7177
{
7278
}
7379
static inline void bpf_jit_free(struct sk_filter *fp)
7480
{
81+
kfree(fp);
7582
}
7683
#define SK_RUN_FILTER(FILTER, SKB) sk_run_filter(SKB, FILTER->insns)
7784
#endif

‎include/net/sock.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1630,16 +1630,14 @@ static inline void sk_filter_release(struct sk_filter *fp)
16301630

16311631
static inline void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp)
16321632
{
1633-
unsigned int size = sk_filter_len(fp);
1634-
1635-
atomic_sub(size, &sk->sk_omem_alloc);
1633+
atomic_sub(sk_filter_size(fp->len), &sk->sk_omem_alloc);
16361634
sk_filter_release(fp);
16371635
}
16381636

16391637
static inline void sk_filter_charge(struct sock *sk, struct sk_filter *fp)
16401638
{
16411639
atomic_inc(&fp->refcnt);
1642-
atomic_add(sk_filter_len(fp), &sk->sk_omem_alloc);
1640+
atomic_add(sk_filter_size(fp->len), &sk->sk_omem_alloc);
16431641
}
16441642

16451643
/*

‎net/core/filter.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,6 @@ void sk_filter_release_rcu(struct rcu_head *rcu)
644644
struct sk_filter *fp = container_of(rcu, struct sk_filter, rcu);
645645

646646
bpf_jit_free(fp);
647-
kfree(fp);
648647
}
649648
EXPORT_SYMBOL(sk_filter_release_rcu);
650649

@@ -683,7 +682,7 @@ int sk_unattached_filter_create(struct sk_filter **pfp,
683682
if (fprog->filter == NULL)
684683
return -EINVAL;
685684

686-
fp = kmalloc(fsize + sizeof(*fp), GFP_KERNEL);
685+
fp = kmalloc(sk_filter_size(fprog->len), GFP_KERNEL);
687686
if (!fp)
688687
return -ENOMEM;
689688
memcpy(fp->insns, fprog->filter, fsize);
@@ -723,6 +722,7 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
723722
{
724723
struct sk_filter *fp, *old_fp;
725724
unsigned int fsize = sizeof(struct sock_filter) * fprog->len;
725+
unsigned int sk_fsize = sk_filter_size(fprog->len);
726726
int err;
727727

728728
if (sock_flag(sk, SOCK_FILTER_LOCKED))
@@ -732,11 +732,11 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
732732
if (fprog->filter == NULL)
733733
return -EINVAL;
734734

735-
fp = sock_kmalloc(sk, fsize+sizeof(*fp), GFP_KERNEL);
735+
fp = sock_kmalloc(sk, sk_fsize, GFP_KERNEL);
736736
if (!fp)
737737
return -ENOMEM;
738738
if (copy_from_user(fp->insns, fprog->filter, fsize)) {
739-
sock_kfree_s(sk, fp, fsize+sizeof(*fp));
739+
sock_kfree_s(sk, fp, sk_fsize);
740740
return -EFAULT;
741741
}
742742

0 commit comments

Comments
 (0)