[llvm-bugs] [Bug 36573] New: eBPF implementation of __sync_add_and_fetch, __sync_fetch_and_add returns incorrect value

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=36573

            Bug ID: 36573
           Summary: eBPF implementation of __sync_add_and_fetch,
                    __sync_fetch_and_add returns incorrect value
           Product: clang
           Version: 5.0
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: LLVM Codegen
          Assignee: unassignedclangbugs at nondot.org
          Reporter: palvarez at akamai.com
                CC: llvm-bugs at lists.llvm.org

Created attachment 19983
  --> https://bugs.llvm.org/attachment.cgi?id=19983&action=edit
output of clang make

Whem compiling eBPF programs using clang,

__sync_fetch_and_add(int * value, int addby) should return *value before the
addition. In fact, it returns addby.

Similarly, 

__sync_add_and_fetch(int * value, int addby) should return *value after the
addition. In fact, it returns addby.

I have not tested other __sync functions.

The reason this is a problem is that it makes it impossible to use a
BPF_MAP_TYPE_ARRAY to hold atomic counters. Here's what man bpf(2) says:


              *  map_update_elem() replaces elements in a nonatomic fashion;
                 for atomic updates, a hash-table map should be used
                 instead.  There is however one special case that can also
                 be used with arrays: the atomic built-in
                 __sync_fetch_and_add() can be used on 32 and 64 bit atomic
                 counters.  For example, it can be applied on the whole
                 value itself if it represents a single counter, or in case
                 of a structure containing multiple counters, it could be
                 used on individual counters.  This is quite often useful
                 for aggregation and accounting of events.

Unfortunately, the fact that the return value is incorrect makes it not
possible to use that return value correctly (say, as an index into another
map). One has to look up the value outside of the __syncXXX call, so race
conditions can occur.

The following program demonstrates this problem. Attachments include 
* compile output with clang 5.0.1 
* kernel logs produced by inserting the program.
* bytecode produced by loading the program with 'tc ... verbose'

Note: the program was compiled within kernel 4.9.0 sources

(ALSI9)palvarez at padesk:~/workspaces/kernel/akamai/linux/samples/bpf$ cat
clsact_get_packet.c
/* Copyright (c) 2016 Facebook
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of version 2 of the GNU General Public
 * License as published by the Free Software Foundation.
 */
#define KBUILD_MODNAME "foo"
#include <linux/ip.h>
#include <linux/ipv6.h>
#include <linux/in.h>
#include <linux/tcp.h>
#include <uapi/linux/bpf.h>
#include <net/ip.h>
#include "bpf_helpers.h"

#define PIN_GLOBAL_NS             2
struct bpf_elf_map {
  __u32 type;
  __u32 size_key;
  __u32 size_value;
  __u32 max_elem;
  __u32 flags;
  __u32 id;
  __u32 pinning;
};

#define bpf_debug(fmt, ...)                                             \
  ({                                                      \
   char ____fmt[] = fmt;                           \
   bpf_trace_printk(____fmt, sizeof(____fmt),      \
##__VA_ARGS__);                    \
   })

//Holds persistent variables across invocations
//index 0: ring buffer counter
struct bpf_elf_map SEC("maps") out_vars_map = {
  .type = BPF_MAP_TYPE_ARRAY,
  .size_key = sizeof(int),
  .size_value = sizeof(int),
  .pinning = PIN_GLOBAL_NS,
  .max_elem = 8,
};

SEC("getpacket")
int handle_egress(struct __sk_buff *skb)
{
  int index = 0;
  int * value = bpf_map_lookup_elem(&out_vars_map, &index);
  index = 1;
  int * value2 = bpf_map_lookup_elem(&out_vars_map, &index);
  if (!value || !value2) { bpf_debug("should be impossible, but check anyway
for verifier"); return TC_ACT_OK; }

  int before = *value;
  int ret = __sync_fetch_and_add(value, 3);
  bpf_debug("at index 0 before %d now %d return from __sync_fetch_and_add(3)
%d\n", before, *value, ret);
  before = *value2;
  ret = __sync_add_and_fetch(value2, 2);
  bpf_debug("at index 1 before %d now %d return from __sync_add_and_fetch(2)
%d\n", before, *value2, ret);
  return TC_ACT_OK;
}

char _license[] SEC("license") = "GPL";

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20180302/d96e0d92/attachment-0001.html>