On Fri, Dec 16, 2022 at 6:15 PM Hou Tao <houtao1@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 12/14/2022 6:38 PM, xiangxia.m.yue@xxxxxxxxx wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@xxxxxxxxx>
> >
> > The deadlock still may occur while accessed in NMI and non-NMI
> > context. Because in NMI, we still may access the same bucket but with
> > different map_locked index.
> >
> > For example, on the same CPU, .max_entries = 2, we update the hash map,
> > with key = 4, while running bpf prog in NMI nmi_handle(), to update
> > hash map with key = 20, so it will have the same bucket index but have
> > different map_locked index.
> >
> > To fix this issue, using min mask to hash again.
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@xxxxxxxxx>
> > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > Cc: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > Cc: Martin KaFai Lau <martin.lau@xxxxxxxxx>
> > Cc: Song Liu <song@xxxxxxxxxx>
> > Cc: Yonghong Song <yhs@xxxxxx>
> > Cc: John Fastabend <john.fastabend@xxxxxxxxx>
> > Cc: KP Singh <kpsingh@xxxxxxxxxx>
> > Cc: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > Cc: Hao Luo <haoluo@xxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Hou Tao <houtao1@xxxxxxxxxx>
> > ---
> > kernel/bpf/hashtab.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index 5aa2b5525f79..8b25036a8690 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -152,7 +152,7 @@ static inline int htab_lock_bucket(const struct bpf_htab *htab,
> > {
> > unsigned long flags;
> >
> > - hash = hash & HASHTAB_MAP_LOCK_MASK;
> > + hash = hash & min(HASHTAB_MAP_LOCK_MASK, htab->n_buckets -1);
> There is warning for kernel test robot and it seems that min_t(...) is required
I will send v2 soon. Thanks.
> here.
>
> Otherwise, this patch looks good to me, so:
>
> Acked-by: Hou Tao <houtao1@xxxxxxxxxx>
> >
> > preempt_disable();
> > if (unlikely(__this_cpu_inc_return(*(htab->map_locked[hash])) != 1)) {
> > @@ -171,7 +171,7 @@ static inline void htab_unlock_bucket(const struct bpf_htab *htab,
> > struct bucket *b, u32 hash,
> > unsigned long flags)
> > {
> > - hash = hash & HASHTAB_MAP_LOCK_MASK;
> > + hash = hash & min(HASHTAB_MAP_LOCK_MASK, htab->n_buckets -1);
> > raw_spin_unlock_irqrestore(&b->raw_lock, flags);
> > __this_cpu_dec(*(htab->map_locked[hash]));
> > preempt_enable();
>
--
Best regards, Tonghao
