* Re: [RFC PATCH iproute2] Drop capabilities if not running ip exec vrf with libcap
From: Luca Boccassi @ 2018-03-27 17:43 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, dsahern, luto
In-Reply-To: <20180327101519.473a1372@xeon-e3>
[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]
On Tue, 2018-03-27 at 10:15 -0700, Stephen Hemminger wrote:
> On Tue, 27 Mar 2018 17:24:19 +0100
> Luca Boccassi <bluca@debian.org> wrote:
>
> > ip vrf exec requires root or CAP_NET_ADMIN, CAP_SYS_ADMIN and
> > CAP_DAC_OVERRIDE. It is not possible to run unprivileged commands
> > like
> > ping as non-root or non-cap-enabled due to this requirement.
> > To allow users and administrators to safely add the required
> > capabilities to the binary, drop all capabilities on start if not
> > invoked with "vrf exec".
> > Update the manpage with the requirements.
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
>
> Gets a little messy, but don't have a better answer.
> When a command like iproute gets involved in security policy things
> I become concerned that it may have unexpected consequences.
Yeah I understand. It requires an explicit action by the sysadmin, to
give you plausible deniability :-)
I've seen changes to let BPF permissions be managed via an LSM (I think
SELinux support is already merged in 4.15), so perhaps one day we'll be
able to do the whole shebang (subdir in /sys + load bpf + manipulate
cgroup) in a more fine-grained way, but for now I think this will do.
I'll send v1 shortly with the change asked by David.
--
Kind regards,
Luca Boccassi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH iproute2 v1] Drop capabilities if not running ip exec vrf with libcap
From: Luca Boccassi @ 2018-03-27 17:48 UTC (permalink / raw)
To: netdev; +Cc: dsahern, luto, stephen, Luca Boccassi
In-Reply-To: <20180327162419.8962-1-bluca@debian.org>
ip vrf exec requires root or CAP_NET_ADMIN, CAP_SYS_ADMIN and
CAP_DAC_OVERRIDE. It is not possible to run unprivileged commands like
ping as non-root or non-cap-enabled due to this requirement.
To allow users and administrators to safely add the required
capabilities to the binary, drop all capabilities on start if not
invoked with "vrf exec".
Update the manpage with the requirements.
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
Changes since RFC: moved drop_cap to lib/util.c to call it from
ipvrf.c after vrf_switch, which is the function that requires the
additional permissions, has finished.
configure | 17 +++++++++++++++++
include/utils.h | 2 ++
ip/ip.c | 12 ++++++++++++
ip/ipvrf.c | 2 ++
lib/utils.c | 21 +++++++++++++++++++++
man/man8/ip-vrf.8 | 8 ++++++++
6 files changed, 62 insertions(+)
diff --git a/configure b/configure
index f7c2d7a7..5ef5cd4c 100755
--- a/configure
+++ b/configure
@@ -336,6 +336,20 @@ EOF
rm -f $TMPDIR/strtest.c $TMPDIR/strtest
}
+check_cap()
+{
+ if ${PKG_CONFIG} libcap --exists
+ then
+ echo "HAVE_CAP:=y" >>$CONFIG
+ echo "yes"
+
+ echo 'CFLAGS += -DHAVE_LIBCAP' `${PKG_CONFIG} libcap --cflags` >>$CONFIG
+ echo 'LDLIBS +=' `${PKG_CONFIG} libcap --libs` >> $CONFIG
+ else
+ echo "no"
+ fi
+}
+
quiet_config()
{
cat <<EOF
@@ -410,6 +424,9 @@ check_berkeley_db
echo -n "need for strlcpy: "
check_strlcpy
+echo -n "libcap support: "
+check_cap
+
echo >> $CONFIG
echo "%.o: %.c" >> $CONFIG
echo ' $(QUIET_CC)$(CC) $(CFLAGS) $(EXTRA_CFLAGS) -c -o $@ $<' >> $CONFIG
diff --git a/include/utils.h b/include/utils.h
index 0394268e..e7bffe8a 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -299,4 +299,6 @@ size_t strlcpy(char *dst, const char *src, size_t size);
size_t strlcat(char *dst, const char *src, size_t size);
#endif
+void drop_cap(void);
+
#endif /* __UTILS_H__ */
diff --git a/ip/ip.c b/ip/ip.c
index e0cd96cb..e716fed8 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -174,6 +174,18 @@ int main(int argc, char **argv)
char *batch_file = NULL;
int color = 0;
+ /* to run vrf exec without root, capabilities might be set, drop them
+ * if not needed as the first thing.
+ * execv will drop them for the child command.
+ * vrf exec requires:
+ * - cap_dac_override to create the cgroup subdir in /sys
+ * - cap_sys_admin to load the BPF program
+ * - cap_net_admin to set the socket into the cgroup
+ */
+ if (argc < 3 || strcmp(argv[1], "vrf") != 0 ||
+ strcmp(argv[2], "exec") != 0)
+ drop_cap();
+
basename = strrchr(argv[0], '/');
if (basename == NULL)
basename = argv[0];
diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index f9277e1e..8a6b7f97 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -436,6 +436,8 @@ out2:
out:
free(mnt);
+ drop_cap();
+
return rc;
}
diff --git a/lib/utils.c b/lib/utils.c
index eba4fa74..d20cc594 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -30,6 +30,9 @@
#include <time.h>
#include <sys/time.h>
#include <errno.h>
+#ifdef HAVE_LIBCAP
+#include <sys/capability.h>
+#endif
#include "rt_names.h"
#include "utils.h"
@@ -1482,3 +1485,21 @@ size_t strlcat(char *dst, const char *src, size_t size)
return dlen + strlcpy(dst + dlen, src, size - dlen);
}
#endif
+
+void drop_cap(void)
+{
+#ifdef HAVE_LIBCAP
+ /* don't harmstring root/sudo */
+ if (getuid() != 0 && geteuid() != 0) {
+ cap_t capabilities;
+ capabilities = cap_get_proc();
+ if (!capabilities)
+ exit(EXIT_FAILURE);
+ if (cap_clear(capabilities) != 0)
+ exit(EXIT_FAILURE);
+ if (cap_set_proc(capabilities) != 0)
+ exit(EXIT_FAILURE);
+ cap_free(capabilities);
+ }
+#endif
+}
diff --git a/man/man8/ip-vrf.8 b/man/man8/ip-vrf.8
index 18789339..1a42cebe 100644
--- a/man/man8/ip-vrf.8
+++ b/man/man8/ip-vrf.8
@@ -63,6 +63,14 @@ a VRF other than the default VRF (main table). A command can be run against
the default VRF by passing the "default" as the VRF name. This is useful if
the current shell is associated with another VRF (e.g, Management VRF).
+This command requires the system to be booted with cgroup v2 (e.g. with systemd,
+add systemd.unified_cgroup_hierarchy=1 to the kernel command line).
+
+This command also requires to be ran as root or with the CAP_SYS_ADMIN,
+CAP_NET_ADMIN and CAP_DAC_OVERRIDE capabilities. If built with libcap and if
+capabilities are added to the ip binary program via setcap, the program will
+drop them as the first thing when invoked, unless the command is vrf exec.
+
.TP
.B ip vrf identify [PID] - Report VRF association for process
.sp
--
2.14.2
^ permalink raw reply related
* Re: [PATCH net-next] qed*: Utilize FW 8.33.11.0
From: Leon Romanovsky @ 2018-03-27 17:50 UTC (permalink / raw)
To: Kalderon, Michal
Cc: Jason Gunthorpe, davem@davemloft.net, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org,
Tayar, Tomer, Rangankar, Manish, Elior, Ariel
In-Reply-To: <CY1PR0701MB201259F4DC154EFA6AE90E6F88AC0@CY1PR0701MB2012.namprd07.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 2613 bytes --]
On Tue, Mar 27, 2018 at 05:41:51PM +0000, Kalderon, Michal wrote:
> > From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
> > Sent: Tuesday, March 27, 2018 12:18 AM
> > > diff --git a/drivers/infiniband/hw/qedr/main.c
> > > b/drivers/infiniband/hw/qedr/main.c
> > > index db4bf97..7dbbe6d 100644
> > > +++ b/drivers/infiniband/hw/qedr/main.c
> > > @@ -51,6 +51,7 @@
> > > MODULE_DESCRIPTION("QLogic 40G/100G ROCE Driver");
> > > MODULE_AUTHOR("QLogic Corporation"); MODULE_LICENSE("Dual
> > BSD/GPL");
> > > +MODULE_VERSION(QEDR_MODULE_VERSION);
> > >
> > > #define QEDR_WQ_MULTIPLIER_DFT (3)
> > >
> > > diff --git a/drivers/infiniband/hw/qedr/qedr.h
> > > b/drivers/infiniband/hw/qedr/qedr.h
> > > index 86d4511..ab0d411 100644
> > > +++ b/drivers/infiniband/hw/qedr/qedr.h
> > > @@ -43,6 +43,8 @@
> > > #include "qedr_hsi_rdma.h"
> > >
> > > #define QEDR_NODE_DESC "QLogic 579xx RoCE HCA"
> > > +#define QEDR_MODULE_VERSION "8.33.11.20"
> > > +
> >
> > I thought we had a general prohibition against versions like this in mainline
> > drivers? And what does this hunk have to do with supporting new firmware?
> >
> > Jason
> I'm assuming you refer only to rdma in regards to version prohibition right ? as looking at all other vendors
> (including Mellanox) all have module versions under net/ why is rdma different in this way ?
> I now searched back mails on the topic and found an email from Leon where he stated:
> " I am strongly against module versions. You should rely on official kernel version."
> But it's not always the inbox driver that is installed or probed, the kernel version is not enough.
> Given different distros, vanilla kernels, out of box drivers, etc... it is essential for us that based on
> logs And modinfo we can determine the qed* drivers that are running.
We actually stopped to maintain driver versions, just ensure that inbox,
upstream and MLNX_OFED have different names.
The discussion thread is here
https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-June/004426.html
https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-June/004441.html
>
> We have received complaints that our qedr module doesn't have a version whereas all of our other
> components do (qed, qede, qedi, qedf). We decided to add the qedr version with the next version
> update to align with the rest of the components.
>
> We can move the driver version bump into a different commit for all components, just made
> sense to Add it to this one as it is the root of the version update.
> Let me know if you think it is essential and I'll make the change for v2
>
> Thanks,
> Michal
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v13 net-next 09/12] crypto: chtls - Inline TLS record Tx
From: Stefano Brivio @ 2018-03-27 17:51 UTC (permalink / raw)
To: Atul Gupta
Cc: davem, herbert, davejwatson, sd, linux-crypto, netdev, werner,
leedom, swise, indranil, ganeshgr
In-Reply-To: <1522172201-7629-10-git-send-email-atul.gupta@chelsio.com>
On Tue, 27 Mar 2018 23:06:38 +0530
Atul Gupta <atul.gupta@chelsio.com> wrote:
> +static u8 tcp_state_to_flowc_state(u8 state)
> +{
> + u8 ret = FW_FLOWC_MNEM_TCPSTATE_ESTABLISHED;
> +
> + switch (state) {
> + case TCP_ESTABLISHED:
> + ret = FW_FLOWC_MNEM_TCPSTATE_ESTABLISHED;
> + break;
> + case TCP_CLOSE_WAIT:
> + ret = FW_FLOWC_MNEM_TCPSTATE_CLOSEWAIT;
> + break;
> + case TCP_FIN_WAIT1:
> + ret = FW_FLOWC_MNEM_TCPSTATE_FINWAIT1;
> + break;
> + case TCP_CLOSING:
> + ret = FW_FLOWC_MNEM_TCPSTATE_CLOSING;
> + break;
> + case TCP_LAST_ACK:
> + ret = FW_FLOWC_MNEM_TCPSTATE_LASTACK;
> + break;
> + case TCP_FIN_WAIT2:
> + ret = FW_FLOWC_MNEM_TCPSTATE_FINWAIT2;
> + break;
Can't you just return those values right away instead?
> [...]
>
> +static u64 tlstx_seq_number(struct chtls_hws *hws)
> +{
> + return hws->tx_seq_no++;
> +}
The name of this function, as I also had commented earlier, is
misleading, because you are also incrementing the sequence number.
> [...]
>
> +static void mark_urg(struct tcp_sock *tp, int flags,
> + struct sk_buff *skb)
> +{
> + if (unlikely(flags & MSG_OOB)) {
> + tp->snd_up = tp->write_seq;
> + ULP_SKB_CB(skb)->flags = ULPCB_FLAG_URG |
> + ULPCB_FLAG_BARRIER |
> + ULPCB_FLAG_NO_APPEND |
> + ULPCB_FLAG_NEED_HDR;
Is this indentation the result of a previous 'if' clause which is now
gone?
> [...]
>
> +/*
> + * Returns true if a TCP socket is corked.
> + */
> +static int corked(const struct tcp_sock *tp, int flags)
> +{
> + return (flags & MSG_MORE) | (tp->nonagle & TCP_NAGLE_CORK);
I guess you meant || here. Shouldn't this be a bool?
> +}
> +
> +/*
> + * Returns true if a send should try to push new data.
> + */
> +static int send_should_push(struct sock *sk, int flags)
> +{
> + return should_push(sk) && !corked(tcp_sk(sk), flags);
> +}
If it returns true, I guess it should be a bool.
> [...]
--
Stefano
^ permalink raw reply
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"
From: Stephen Hemminger @ 2018-03-27 17:58 UTC (permalink / raw)
To: Alexander Zubkov; +Cc: Serhey Popovych, Luca Boccassi, netdev@vger.kernel.org
In-Reply-To: <998171522172008@web36j.yandex.ru>
On Tue, 27 Mar 2018 19:33:28 +0200
Alexander Zubkov <green@msu.ru> wrote:
> master before merging revert + my recent patch (1) should work. Or you mean to prepare patch to change new master to desired state? I can do it.
>
> 1) https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/patch/?id=7696f1097f79be2ce5984a8a16103fd17391cac2
>
> 27.03.2018, 19:00, "Stephen Hemminger" <stephen@networkplumber.org>:
> > On Tue, 27 Mar 2018 18:29:31 +0200
> > Alexander Zubkov <green@msu.ru> wrote:
> >
> >> Hi Stephen,
> >>
> >> Looks like the new patch was applied after the revert of original patch and fix patch for 4.15 branch. Which is not correct and I did not test it. This is how patches were designed:
> >> 1) your revert patch - rolls back 4.15 branch to old behaviour by reverting the original patch
> >> 2) my patch for 4.15 - fixes problem is 4.15 branch, it does not require revert patch, it is an alternative solution for the problem, it is designed solely for version 4.15
> >> 3) my patch for master - fixes problem, it requires neither revert patch nor my patch for 4.15, it is standalone patch designed to do things right in master branch
> >>
> >> 27.03.2018, 18:01, "Stephen Hemminger" <stephen@networkplumber.org>:
> >> > On Wed, 14 Mar 2018 21:26:40 +0100
> >> > Alexander Zubkov <green@msu.ru> wrote:
> >> >
> >> >> Hello,
> >> >>
> >> >> For example, it can be fixed in such way (patch is below):
> >> >> - split handling of default and all/any
> >> >> - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default
> >> >> - and AF_UNSPEC for all/any
> >> >> In this case "ip route show default" shows only default route and "ip
> >> >> route show all" shows all routes. And both also work when family (-4 or
> >> >> -6) is specified.
> >> >> Serhey, does it goes in line with what you wanted to achieve? Because I
> >> >> do not know - may be there are reasons why all/any should be provided
> >> >> with specific family. If you think this solution is suitable, I'll do
> >> >> some additional tests and package the patch in a proper way for this
> >> >> mailing list.
> >> >> And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both
> >> >> branches. May be someone have some additional thoughts on that?
> >> >
> >> > I applied this to master.
> >> >
> >> > We can work on the other cases after that.
> >
> > Please send the update back to what works.
Make patches against current master.
For visible repositories, I prefer to only move forward and not rollback.
So you can send a revert patch than new code if that is easier.
^ permalink raw reply
* Re: [PATCH v13 net-next 07/12] crypto: chtls - Program the TLS session Key
From: Stefano Brivio @ 2018-03-27 18:16 UTC (permalink / raw)
To: Atul Gupta
Cc: davem, herbert, davejwatson, sd, linux-crypto, netdev, werner,
leedom, swise, indranil, ganeshgr
In-Reply-To: <1522172201-7629-8-git-send-email-atul.gupta@chelsio.com>
On Tue, 27 Mar 2018 23:06:36 +0530
Atul Gupta <atul.gupta@chelsio.com> wrote:
> +static void __set_tcb_field(struct sock *sk, struct sk_buff *skb, u16 word,
> + u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
Please use reverse christmas tree style for variable declarations. If
needed, do the assignments later on.
> [...]
>
> +static int chtls_set_tcb_field(struct sock *sk, u16 word, u64 mask, u64 val)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct cpl_set_tcb_field *req;
> + struct ulptx_idata *sc;
> + struct sk_buff *skb;
> + int ret;
> + unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> + unsigned int credits_needed = DIV_ROUND_UP(wrlen, 16);
Same here.
> [...]
>
> +static int get_new_keyid(struct chtls_sock *csk, u32 optname)
> +{
> + struct chtls_hws *hws = &csk->tlshws;
> + struct net_device *dev = csk->egress_dev;
> + struct adapter *adap = netdev2adap(dev);
> + struct chtls_dev *cdev = csk->cdev;
> + int keyid;
Same here.
> +
> + spin_lock_bh(&cdev->kmap.lock);
> + keyid = find_first_zero_bit(cdev->kmap.addr, cdev->kmap.size);
> + if (keyid < cdev->kmap.size) {
> + __set_bit(keyid, cdev->kmap.addr);
> + if (optname == TLS_RX)
> + hws->rxkey = keyid;
> + else
> + hws->txkey = keyid;
> + atomic_inc(&adap->chcr_stats.tls_key);
> + } else {
> + keyid = -1;
> + }
> + spin_unlock_bh(&cdev->kmap.lock);
> + return keyid;
> +}
> +
> +void free_tls_keyid(struct sock *sk)
> +{
> + struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> + struct net_device *dev = csk->egress_dev;
> + struct adapter *adap = netdev2adap(dev);
> + struct chtls_dev *cdev = csk->cdev;
> + struct chtls_hws *hws = &csk->tlshws;
Same here.
> [...]
>
> +static int chtls_key_info(struct chtls_sock *csk,
> + struct _key_ctx *kctx,
> + u32 keylen, u32 optname)
> +{
> + unsigned char key[CHCR_KEYCTX_CIPHER_KEY_SIZE_256];
> + struct crypto_cipher *cipher;
> + struct tls12_crypto_info_aes_gcm_128 *gcm_ctx =
> + (struct tls12_crypto_info_aes_gcm_128 *)
> + &csk->tlshws.crypto_info;
> + unsigned char ghash_h[AEAD_H_SIZE];
> + int ck_size, key_ctx_size;
> + int ret;
Same here.
> [...]
>
> +int chtls_setkey(struct chtls_sock *csk, u32 keylen, u32 optname)
> +{
> + struct chtls_dev *cdev = csk->cdev;
> + struct sock *sk = csk->sk;
> + struct tls_key_req *kwr;
> + struct _key_ctx *kctx;
> + struct sk_buff *skb;
> + int wrlen, klen, len;
> + int keyid;
> + int kaddr;
> + int ret = 0;
Same here.
> [...]
--
Stefano
^ permalink raw reply
* Re: [PATCH v13 net-next 01/12] tls: support for Inline tls record
From: Stefano Brivio @ 2018-03-27 18:23 UTC (permalink / raw)
To: Atul Gupta
Cc: davem, herbert, davejwatson, sd, linux-crypto, netdev, werner,
leedom, swise, indranil, ganeshgr
In-Reply-To: <1522172201-7629-2-git-send-email-atul.gupta@chelsio.com>
On Tue, 27 Mar 2018 23:06:30 +0530
Atul Gupta <atul.gupta@chelsio.com> wrote:
> +static struct tls_context *create_ctx(struct sock *sk)
> +{
> + struct inet_connection_sock *icsk = inet_csk(sk);
> + struct tls_context *ctx;
> +
> + /* allocate tls context */
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return NULL;
> +
> + icsk->icsk_ulp_data = ctx;
> + return ctx;
> +}
>
> [...]
>
> static int tls_init(struct sock *sk)
> {
> int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
> - struct inet_connection_sock *icsk = inet_csk(sk);
> struct tls_context *ctx;
> int rc = 0;
>
> + if (tls_hw_prot(sk))
> + goto out;
> +
> /* The TLS ulp is currently supported only for TCP sockets
> * in ESTABLISHED state.
> * Supporting sockets in LISTEN state will require us
> @@ -530,12 +624,11 @@ static int tls_init(struct sock *sk)
> return -ENOTSUPP;
>
> /* allocate tls context */
> - ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> + ctx = create_ctx(sk);
> if (!ctx) {
> rc = -ENOMEM;
> goto out;
> }
> - icsk->icsk_ulp_data = ctx;
Why are you changing this?
This is now equivalent to the original implementation, except that you
are "hiding" the assignment of icsk->icsk_ulp_data into a function named
"create_ctx".
Please also note that you are duplicating the "allocate tls context"
comment.
--
Stefano
^ permalink raw reply
* [PATCH net] hv_netvsc: enable multicast if necessary
From: Stephen Hemminger @ 2018-03-27 18:28 UTC (permalink / raw)
To: davem; +Cc: netdev, Stephen Hemminger
My recent change to netvsc drive in how receive flags are handled
broke multicast. The Hyper-v/Azure virtual interface there is not a
multicast filter list, filtering is only all or none. The driver must
enable all multicast if any multicast address is present.
Fixes: 009f766ca238 ("hv_netvsc: filter multicast/broadcast")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/rndis_filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 00ec80c23fe5..8799b0590adc 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -864,7 +864,7 @@ static void rndis_set_multicast(struct work_struct *w)
if (flags & IFF_PROMISC) {
filter = NDIS_PACKET_TYPE_PROMISCUOUS;
} else {
- if (flags & IFF_ALLMULTI)
+ if (!netdev_mc_empty(rdev->ndev) || (flags & IFF_ALLMULTI))
filter |= NDIS_PACKET_TYPE_ALL_MULTICAST;
if (flags & IFF_BROADCAST)
filter |= NDIS_PACKET_TYPE_BROADCAST;
--
2.16.2
^ permalink raw reply related
* Re: [iproute2 1/1] ss: Add support for TIPC socket diag in ss tool
From: Stephen Hemminger @ 2018-03-27 18:39 UTC (permalink / raw)
To: GhantaKrishnamurthy MohanKrishna
Cc: tipc-discussion, jon.maloy, maloy, ying.xue, netdev,
Parthasarathy Bhuvaragan
In-Reply-To: <1521813662-9954-2-git-send-email-mohan.krishna.ghanta.krishnamurthy@ericsson.com>
On Fri, 23 Mar 2018 15:01:02 +0100
GhantaKrishnamurthy MohanKrishna <mohan.krishna.ghanta.krishnamurthy@ericsson.com> wrote:
> For iproute 4.x
> Allow TIPC socket statistics to be dumped with --tipc
> and tipc specific info with --tipcinfo.
>
> Acked-by: Jon Maloy <jon.maloy@ericsson.com>
> Signed-off-by: GhantaKrishnamurthy MohanKrishna <mohan.krishna.ghanta.krishnamurthy@ericsson.com>
> Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@gmail.com>
> ---
> misc/ss.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 164 insertions(+), 2 deletions(-)
>
> diff --git a/misc/ss.c b/misc/ss.c
> index e047f9c04582..812f45717af9 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -45,6 +45,10 @@
> #include <linux/netlink_diag.h>
> #include <linux/sctp.h>
> #include <linux/vm_sockets_diag.h>
> +#include <linux/net.h>
> +#include <linux/tipc.h>
> +#include <linux/tipc_netlink.h>
> +#include <linux/tipc_sockets_diag.h>
This needs to be against iproute2-next, not master since there is no tipc_sockets_diag.h
in Linus's tree.
All the header files for iproute2 include/uapi/linux come from upstream kernel
via 'make headers_install'. Therefore you need to have tipc_sockets_diag.h in
kernel already.
^ permalink raw reply
* Re: [iproute PATCH] ssfilter: Eliminate shift/reduce conflicts
From: Stephen Hemminger @ 2018-03-27 18:42 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev
In-Reply-To: <20180324174514.17840-1-phil@nwl.cc>
On Sat, 24 Mar 2018 18:45:14 +0100
Phil Sutter <phil@nwl.cc> wrote:
> The problematic bit was the 'expr: expr expr' rule. Fix this by making
> 'expr' token represent a single filter only and introduce a new token
> 'exprlist' to represent a combination of filters.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
Thanks for fixing this. It was always a nuisance.
^ permalink raw reply
* Re: [iproute PATCH 2/3] ss: Put filter DB parsing into a separate function
From: Stephen Hemminger @ 2018-03-27 18:46 UTC (permalink / raw)
To: Phil Sutter; +Cc: netdev
In-Reply-To: <20180324181811.22615-3-phil@nwl.cc>
On Sat, 24 Mar 2018 19:18:10 +0100
Phil Sutter <phil@nwl.cc> wrote:
> +#define ENTRY(name, ...) { #name, { __VA_ARGS__, MAX_DB }}
> + ENTRY(all, UDP_DB, DCCP_DB, TCP_DB, RAW_DB, \
> + UNIX_ST_DB, UNIX_DG_DB, UNIX_SQ_DB, \
> + PACKET_R_DB, PACKET_DG_DB, NETLINK_DB, \
> + SCTP_DB, VSOCK_ST_DB, VSOCK_DG_DB),
Checkpatch complains that line continuations are not necessary here;
and it is right. Macro usage can cross lines.
^ permalink raw reply
* Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT
From: Alexei Starovoitov @ 2018-03-27 18:45 UTC (permalink / raw)
To: Steven Rostedt
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers, Kees Cook
In-Reply-To: <20180327130211.284c8924@gandalf.local.home>
On 3/27/18 10:02 AM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 19:47:03 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>
>> Ctrl-C of tracing daemon or cmdline tool that uses this feature
>> will automatically detach bpf program, unload it and
>> unregister tracepoint probe.
>>
>> On the kernel side for_each_kernel_tracepoint() is used
>
> You need to update the change log to state
> kernel_tracepoint_find_by_name().
ahh. right. will do.
>> +#undef DECLARE_EVENT_CLASS
>> +#define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
>> +/* no 'static' here. The bpf probe functions are global */ \
>> +notrace void \
>
> I'm curious to why you have notrace here? Since it is separate from
> perf and ftrace, for debugging purposes, it could be useful to allow
> function tracing to this function.
To avoid unnecessary overhead. And I don't think it's useful to trace
them. They're tiny jump functions of one or two instructions.
Really no point wasting mentry on them.
>> +static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
>> +{
>> + struct bpf_raw_tracepoint *raw_tp;
>> + struct tracepoint *tp;
>> + struct bpf_prog *prog;
>> + char tp_name[128];
>> + int tp_fd, err;
>> +
>> + if (strncpy_from_user(tp_name, u64_to_user_ptr(attr->raw_tracepoint.name),
>> + sizeof(tp_name) - 1) < 0)
>> + return -EFAULT;
>> + tp_name[sizeof(tp_name) - 1] = 0;
>> +
>> + tp = kernel_tracepoint_find_by_name(tp_name);
>> + if (!tp)
>> + return -ENOENT;
>> +
>> + raw_tp = kmalloc(sizeof(*raw_tp), GFP_USER | __GFP_ZERO);
>
> Please use kzalloc(), instead of open coding the "__GPF_ZERO"
right. will do
>
> Could you add some comments here to explain what the below is doing.
To write a proper comment I need to understand it and I don't.
That's the reason why I didn't put in in common header,
because it would require proper comment on what it is and
how one can use it.
I'm expecting Daniel to follow up on this.
>> +#define UNPACK(...) __VA_ARGS__
>> +#define REPEAT_1(FN, DL, X, ...) FN(X)
>> +#define REPEAT_2(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_1(FN, DL, __VA_ARGS__)
>> +#define REPEAT_3(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_2(FN, DL, __VA_ARGS__)
>> +#define REPEAT_4(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_3(FN, DL, __VA_ARGS__)
>> +#define REPEAT_5(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_4(FN, DL, __VA_ARGS__)
>> +#define REPEAT_6(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_5(FN, DL, __VA_ARGS__)
>> +#define REPEAT_7(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_6(FN, DL, __VA_ARGS__)
>> +#define REPEAT_8(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_7(FN, DL, __VA_ARGS__)
>> +#define REPEAT_9(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_8(FN, DL, __VA_ARGS__)
>> +#define REPEAT_10(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_9(FN, DL, __VA_ARGS__)
>> +#define REPEAT_11(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_10(FN, DL, __VA_ARGS__)
>> +#define REPEAT_12(FN, DL, X, ...) FN(X) UNPACK DL REPEAT_11(FN, DL, __VA_ARGS__)
>> +#define REPEAT(X, FN, DL, ...) REPEAT_##X(FN, DL, __VA_ARGS__)
>> +
>> +
>> + snprintf(buf, sizeof(buf), "__bpf_trace_%s", tp->name);
>> + addr = kallsyms_lookup_name(buf);
>> + if (!addr)
>> + return -ENOENT;
>> +
>> + return tracepoint_probe_register(tp, (void *)addr, prog);
>
> You are putting in a hell of a lot of trust with kallsyms returning
> properly. I can see this being very fragile. This is calling a function
> based on the result of kallsyms. I'm sure the security folks would love
> this.
>
> There's a few things to make this a bit more robust. One is to add a
> table that points to all __bpf_trace_* functions, and verify that the
> result from kallsyms is in that table.
>
> Honestly, I think this is too much of a short cut and a hack. I know
> you want to keep it "simple" and save space, but you really should do
> it the same way ftrace and perf do it. That is, create a section and
> have all tracepoints create a structure that holds a pointer to the
> tracepoint and to the bpf probe function. Then you don't even need the
> kernel_tracepoint_find_by_name(), you just iterate over your table and
> you get the tracepoint and the bpf function associated to it.
>
> Relying on kallsyms to return an address to execute is just way too
> extreme and fragile for my liking.
Wasting extra 8bytes * number_of_tracepoints just for lack of trust
in kallsyms doesn't sound like good trade off to me.
If kallsyms are inaccurate all sorts of things will break:
kprobes, livepatch, etc.
I'd rather suggest for ftrace to use kallsyms approach as well
and reduce memory footprint.
^ permalink raw reply
* [resend PATCH] rxrpc: Neaten logging macros and add KERN_DEBUG logging level
From: Joe Perches @ 2018-03-27 18:52 UTC (permalink / raw)
To: David Howells; +Cc: David S. Miller, linux-afs, netdev, linux-kernel
When enabled, the current debug logging does not have a KERN_<LEVEL>.
Add KERN_DEBUG to the logging macros.
Miscellanea:
o Remove #define redundancy and neaten the macros a bit
Signed-off-by: Joe Perches <joe@perches.com>
---
Resend of patch: https://lkml.org/lkml/2017/11/30/573
No change in patch.
David Howells is now a listed maintainer for net/rxrpc/ so he should receive
this patch via get_maintainer
net/rxrpc/ar-internal.h | 75 ++++++++++++++++++-------------------------------
1 file changed, 28 insertions(+), 47 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 416688381eb7..d4b53b2339b3 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -1147,66 +1147,47 @@ static inline bool after_eq(u32 seq1, u32 seq2)
*/
extern unsigned int rxrpc_debug;
-#define dbgprintk(FMT,...) \
- printk("[%-6.6s] "FMT"\n", current->comm ,##__VA_ARGS__)
+#if defined(__KDEBUG) || defined(CONFIG_AF_RXRPC_DEBUG)
+#define dbgprintk(fmt, ...) \
+ printk(KERN_DEBUG "[%-6.6s] " fmt "\n", current->comm, ##__VA_ARGS__)
+#else
+#define dbgprintk(fmt, ...) \
+ no_printk(KERN_DEBUG "[%-6.6s] " fmt "\n", current->comm, ##__VA_ARGS__)
+#endif
-#define kenter(FMT,...) dbgprintk("==> %s("FMT")",__func__ ,##__VA_ARGS__)
-#define kleave(FMT,...) dbgprintk("<== %s()"FMT"",__func__ ,##__VA_ARGS__)
-#define kdebug(FMT,...) dbgprintk(" "FMT ,##__VA_ARGS__)
-#define kproto(FMT,...) dbgprintk("### "FMT ,##__VA_ARGS__)
-#define knet(FMT,...) dbgprintk("@@@ "FMT ,##__VA_ARGS__)
+#define kenter(fmt, ...) dbgprintk("==> %s(" fmt ")", __func__, ##__VA_ARGS__)
+#define kleave(fmt, ...) dbgprintk("<== %s()" fmt "", __func__, ##__VA_ARGS__)
+#define kdebug(fmt, ...) dbgprintk(" " fmt, ##__VA_ARGS__)
+#define kproto(fmt, ...) dbgprintk("### " fmt, ##__VA_ARGS__)
+#define knet(fmt, ...) dbgprintk("@@@ " fmt, ##__VA_ARGS__)
+#if defined(__KDEBUG) || !defined(CONFIG_AF_RXRPC_DEBUG)
+#define _enter(fmt, ...) kenter(fmt, ##__VA_ARGS__)
+#define _leave(fmt, ...) kleave(fmt, ##__VA_ARGS__)
+#define _debug(fmt, ...) kdebug(fmt, ##__VA_ARGS__)
+#define _proto(fmt, ...) kproto(fmt, ##__VA_ARGS__)
+#define _net(fmt, ...) knet(fmt, ##__VA_ARGS__)
-#if defined(__KDEBUG)
-#define _enter(FMT,...) kenter(FMT,##__VA_ARGS__)
-#define _leave(FMT,...) kleave(FMT,##__VA_ARGS__)
-#define _debug(FMT,...) kdebug(FMT,##__VA_ARGS__)
-#define _proto(FMT,...) kproto(FMT,##__VA_ARGS__)
-#define _net(FMT,...) knet(FMT,##__VA_ARGS__)
+#else
-#elif defined(CONFIG_AF_RXRPC_DEBUG)
#define RXRPC_DEBUG_KENTER 0x01
#define RXRPC_DEBUG_KLEAVE 0x02
#define RXRPC_DEBUG_KDEBUG 0x04
#define RXRPC_DEBUG_KPROTO 0x08
#define RXRPC_DEBUG_KNET 0x10
-#define _enter(FMT,...) \
-do { \
- if (unlikely(rxrpc_debug & RXRPC_DEBUG_KENTER)) \
- kenter(FMT,##__VA_ARGS__); \
-} while (0)
-
-#define _leave(FMT,...) \
-do { \
- if (unlikely(rxrpc_debug & RXRPC_DEBUG_KLEAVE)) \
- kleave(FMT,##__VA_ARGS__); \
-} while (0)
-
-#define _debug(FMT,...) \
-do { \
- if (unlikely(rxrpc_debug & RXRPC_DEBUG_KDEBUG)) \
- kdebug(FMT,##__VA_ARGS__); \
-} while (0)
-
-#define _proto(FMT,...) \
-do { \
- if (unlikely(rxrpc_debug & RXRPC_DEBUG_KPROTO)) \
- kproto(FMT,##__VA_ARGS__); \
+#define RXRPC_DEBUG(TYPE, type, fmt, ...) \
+do { \
+ if (unlikely(rxrpc_debug & RXRPC_DEBUG_##TYPE)) \
+ type(fmt, ##__VA_ARGS__); \
} while (0)
-#define _net(FMT,...) \
-do { \
- if (unlikely(rxrpc_debug & RXRPC_DEBUG_KNET)) \
- knet(FMT,##__VA_ARGS__); \
-} while (0)
+#define _enter(fmt, ...) RXRPC_DEBUG(KENTER, kenter, fmt, ##__VA_ARGS__)
+#define _leave(fmt, ...) RXRPC_DEBUG(KLEAVE, kleave, fmt, ##__VA_ARGS__)
+#define _debug(fmt, ...) RXRPC_DEBUG(KDEBUG, kdebug, fmt, ##__VA_ARGS__)
+#define _proto(fmt, ...) RXRPC_DEBUG(KPROTO, kproto, fmt, ##__VA_ARGS__)
+#define _net(fmt, ...) RXRPC_DEBUG(KNET, knet, fmt, ##__VA_ARGS__)
-#else
-#define _enter(FMT,...) no_printk("==> %s("FMT")",__func__ ,##__VA_ARGS__)
-#define _leave(FMT,...) no_printk("<== %s()"FMT"",__func__ ,##__VA_ARGS__)
-#define _debug(FMT,...) no_printk(" "FMT ,##__VA_ARGS__)
-#define _proto(FMT,...) no_printk("### "FMT ,##__VA_ARGS__)
-#define _net(FMT,...) no_printk("@@@ "FMT ,##__VA_ARGS__)
#endif
/*
--
2.15.0
^ permalink raw reply related
* Re: [PATCH iproute2 v1] Drop capabilities if not running ip exec vrf with libcap
From: Stephen Hemminger @ 2018-03-27 18:52 UTC (permalink / raw)
To: Luca Boccassi; +Cc: netdev, dsahern, luto
In-Reply-To: <20180327174855.30497-1-bluca@debian.org>
On Tue, 27 Mar 2018 18:48:55 +0100
Luca Boccassi <bluca@debian.org> wrote:
> ip vrf exec requires root or CAP_NET_ADMIN, CAP_SYS_ADMIN and
> CAP_DAC_OVERRIDE. It is not possible to run unprivileged commands like
> ping as non-root or non-cap-enabled due to this requirement.
> To allow users and administrators to safely add the required
> capabilities to the binary, drop all capabilities on start if not
> invoked with "vrf exec".
> Update the manpage with the requirements.
>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
Applied
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Alexander Duyck @ 2018-03-27 18:54 UTC (permalink / raw)
To: Will Deacon
Cc: Sinan Kaya, Benjamin Herrenschmidt, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org, Linus Torvalds
In-Reply-To: <20180327151029.GB17494@arm.com>
On Tue, Mar 27, 2018 at 8:10 AM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Alex,
>
> On Tue, Mar 27, 2018 at 10:46:58AM -0400, Sinan Kaya wrote:
>> +netdev, +Alex
>>
>> On 3/26/2018 6:00 PM, Benjamin Herrenschmidt wrote:
>> > On Mon, 2018-03-26 at 23:30 +0200, Arnd Bergmann wrote:
>> >> Most of the drivers have a unwound loop with writeq() or something to
>> >>> do it.
>> >>
>> >> But isn't the writeq() barrier much more expensive than anything you'd
>> >> do in function calls?
>> >
>> > It is for us, and will break any write combining.
>> >
>> >>>>> The same document says that _relaxed() does not give that guarentee.
>> >>>>>
>> >>>>> The lwn articule on this went into some depth on the interaction with
>> >>>>> spinlocks.
>> >>>>>
>> >>>>> As far as I can see, containment in a spinlock seems to be the only
>> >>>>> different between writel and writel_relaxed..
>> >>>>
>> >>>> I was always puzzled by this: The intention of _relaxed() on ARM
>> >>>> (where it originates) was to skip the barrier that serializes DMA
>> >>>> with MMIO, not to skip the serialization between MMIO and locks.
>> >>>
>> >>> But that was never a requirement of writel(),
>> >>> Documentation/memory-barriers.txt gives an explicit example demanding
>> >>> the wmb() before writel() for ordering system memory against writel.
>> >
>> > This is a bug in the documentation.
>> >
>> >> Indeed, but it's in an example for when to use dma_wmb(), not wmb().
>> >> Adding Alexander Duyck to Cc, he added that section as part of
>> >> 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
>> >> dma_wmb()"). Also adding the other people that were involved with that.
>> >
>> > Linus himself made it very clear years ago. readl and writel have to
>> > order vs memory accesses.
>> >
>> >>> I actually have no idea why ARM had that barrier, I always assumed it
>> >>> was to give program ordering to the accesses and that _relaxed allowed
>> >>> re-ordering (the usual meaning of relaxed)..
>> >>>
>> >>> But the barrier document makes it pretty clear that the only
>> >>> difference between the two is spinlock containment, and WillD wrote
>> >>> this text, so I belive it is accurate for ARM.
>> >>>
>> >>> Very confusing.
>> >>
>> >> It does mention serialization with both DMA and locks in the
>> >> section about readX_relaxed()/writeX_relaxed(). The part
>> >> about DMA is very clear here, and I must have just forgotten
>> >> the exact semantics with regards to spinlocks. I'm still not
>> >> sure what prevents a writel() from leaking out the end of a
>> >> spinlock section that doesn't happen with writel_relaxed(), since
>> >> the barrier in writel() comes before the access, and the
>> >> spin_unlock() shouldn't affect the external buses.
>> >
>> > So...
>> >
>> > Historically, what happened is that we (we means whoever participated
>> > in the discussion on the list with Linus calling the shots really)
>> > decided that there was no sane way for drivers to understand a world
>> > where readl/writel didn't fully order things vs. memory accesses (ie,
>> > DMA).
>> >
>> > So it should always be correct to do:
>> >
>> > - Write to some in-memory buffer
>> > - writel() to kick the DMA read of that buffer
>> >
>> > without any extra barrier.
>> >
>> > The spinlock situation however got murky. Mostly that came up because
>> > on architecture (I forgot who, might have been ia64) has a hard time
>> > providing that consistency without making writel insanely expensive.
>> >
>> > Thus they created mmiowb whose main purpose was precisely to order
>> > writel with a following spin_unlock.
>> >
>> > I decided not to go down that path on power because getting all drivers
>> > "fixed" to do the right thing was going to be a losing battle, and
>> > instead added per-cpu tracking of writel in order to "escalate" to a
>> > heavier barrier in spin_unlock itself when necessary.
>> >
>> > Now, all this happened more than a decade ago and it's possible that
>> > the understanding or expectations "shifted" over time...
>>
>> Alex is raising concerns on the netdev list.
>>
>> Sinan
>> "We are being told that if you use writel(), then you don't need a wmb() on
>> all architectures."
>>
>> Alex:
>> "I'm not sure who told you that but that is incorrect, at least for
>> x86. If you attempt to use writel() without the wmb() we will have to
>> NAK the patches. We will accept the wmb() with writel_releaxed() since
>> that solves things for ARM."
>>
>> > Jason is seeking behavior clarification for write combined buffers.
>>
>> Alex:
>> "Don't bother. I can tell you right now that for x86 you have to have a
>> wmb() before the writel().
>
> To clarify: are you saying that on x86 you need a wmb() prior to a writel
> if you want that writel to be ordered after prior writes to memory? Is this
> specific to WC memory or some other non-standard attribute?
Note, I am not a CPU guy so this is just my interpretation. It is my
understanding that the wmb(), aka sfence, is needed on x86 to sort out
writes between Write-back(WB) system memory and Strong Uncacheable
(UC) MMIO accesses.
I was hoping to be able to cite something in the software developers
manual (https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf),
but that tends to be pretty vague. I have re-read section 22.34
(volume 3B) several times and I am still not clear on if it says we
need the sfence or not. It is a matter of figuring out what the impact
of store buffers and caching are for WB versus UC memory.
> The only reason we have wmb() inside writel() on arm, arm64 and power is for
> parity with x86 because Linus (CC'd) wanted architectures to order I/O vs
> memory by default so that it was easier to write portable drivers. The
> performance impact of that implicit barrier is non-trivial, but we want the
> driver portability and I went as far as adding generic _relaxed versions for
> the cases where ordering isn't required. You seem to be suggesting that none
> of this is necessary and drivers would already run into problems on x86 if
> they didn't use wmb() explicitly in conjunction with writel, which I find
> hard to believe and is in direct contradiction with the current Linux I/O
> memory model (modulo the broken example in the dma_*mb section of
> memory-barriers.txt).
Is the issue specifically related to memory versus I/O or are there
potential ordering issues for MMIO versus MMIO? I recall when working
on the dma_*mb section that the ARM barriers were much more complex
versus some of the other architectures. One big difference that I can
see for the x86 versus what you define for the "_relaxed" version of
things is the ordering of MMIO operations with respect to locked
transactions. I know x86 forces all MMIO operations to be completed
before you can process any locked operation.
> Has something changed?
>
> Will
As far as I know the code has been this way for a while, something
like 2002, when the barrier was already present in e1000. However
there it was calling out weakly ordered models "such as IA-64". Since
then pretty much all the hardware based network drivers at this point
have similar code floating around with wmb() in place to prevent
issues on weak ordered memory systems.
So in any case we still need to be careful as there are architectures
that are depending on this even if they might not be x86. :-/
- Alex
^ permalink raw reply
* [PATCH] bpf: follow idr code convention
From: Shaohua Li @ 2018-03-27 18:53 UTC (permalink / raw)
To: netdev @ vger . kernel . org
Cc: Kernel Team, Shaohua Li, Daniel Borkmann, Alexei Starovoitov
From: Shaohua Li <shli@fb.com>
Generally we do a preload before doing idr allocation. This also help
improve the allocation success rate in memory pressure.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Shaohua Li <shli@fb.com>
---
kernel/bpf/syscall.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 43f95d1..beab5de 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -203,11 +203,13 @@ static int bpf_map_alloc_id(struct bpf_map *map)
{
int id;
+ idr_preload(GFP_KERNEL);
spin_lock_bh(&map_idr_lock);
id = idr_alloc_cyclic(&map_idr, map, 1, INT_MAX, GFP_ATOMIC);
if (id > 0)
map->id = id;
spin_unlock_bh(&map_idr_lock);
+ idr_preload_end();
if (WARN_ON_ONCE(!id))
return -ENOSPC;
@@ -940,11 +942,13 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
{
int id;
+ idr_preload(GFP_KERNEL);
spin_lock_bh(&prog_idr_lock);
id = idr_alloc_cyclic(&prog_idr, prog, 1, INT_MAX, GFP_ATOMIC);
if (id > 0)
prog->aux->id = id;
spin_unlock_bh(&prog_idr_lock);
+ idr_preload_end();
/* id is in [1, INT_MAX) */
if (WARN_ON_ONCE(!id))
--
2.9.5
^ permalink raw reply related
* Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-27 18:58 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers, Kees Cook
In-Reply-To: <20180327131143.4b83534c@gandalf.local.home>
On Tue, 27 Mar 2018 13:11:43 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 27 Mar 2018 13:02:11 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Honestly, I think this is too much of a short cut and a hack. I know
> > you want to keep it "simple" and save space, but you really should do
> > it the same way ftrace and perf do it. That is, create a section and
> > have all tracepoints create a structure that holds a pointer to the
> > tracepoint and to the bpf probe function. Then you don't even need the
> > kernel_tracepoint_find_by_name(), you just iterate over your table and
> > you get the tracepoint and the bpf function associated to it.
>
> Also, if you do it the perf/ftrace way, you get support for module
> tracepoints pretty much for free. Which would include tracepoints in
> networking code that is loaded by a module.
This doesn't include module code (but that wouldn't be too hard to set
up), but I compiled and booted this. I didn't test if it works (I
didn't have the way to test bpf here). But this patch applies on top of
this patch (patch 8). You can remove patch 7 and fold this into this
patch. And then you can also make the __bpf_trace_* function static.
This would be much more robust and less error prone.
-- Steve
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 1ab0e520d6fc..4fab7392e237 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -178,6 +178,15 @@
#define TRACE_SYSCALLS()
#endif
+#ifdef CONFIG_BPF_EVENTS
+#define BPF_RAW_TP() . = ALIGN(8); \
+ VMLINUX_SYMBOL(__start__bpf_raw_tp) = .; \
+ KEEP(*(__bpf_raw_tp_map)) \
+ VMLINUX_SYMBOL(__stop__bpf_raw_tp) = .;
+#else
+#define BPF_RAW_TP()
+#endif
+
#ifdef CONFIG_SERIAL_EARLYCON
#define EARLYCON_TABLE() STRUCT_ALIGN(); \
VMLINUX_SYMBOL(__earlycon_table) = .; \
@@ -576,6 +585,7 @@
*(.init.rodata) \
FTRACE_EVENTS() \
TRACE_SYSCALLS() \
+ BPF_RAW_TP() \
KPROBE_BLACKLIST() \
ERROR_INJECT_WHITELIST() \
MEM_DISCARD(init.rodata) \
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 399ebe6f90cf..fb4778c0a248 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -470,8 +470,9 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx);
int perf_event_attach_bpf_prog(struct perf_event *event, struct bpf_prog *prog);
void perf_event_detach_bpf_prog(struct perf_event *event);
int perf_event_query_prog_array(struct perf_event *event, void __user *info);
-int bpf_probe_register(struct tracepoint *tp, struct bpf_prog *prog);
-int bpf_probe_unregister(struct tracepoint *tp, struct bpf_prog *prog);
+int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog);
+int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_prog *prog);
+struct bpf_raw_event_map *bpf_find_raw_tracepoint(const char *name);
#else
static inline unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
{
@@ -491,14 +492,18 @@ perf_event_query_prog_array(struct perf_event *event, void __user *info)
{
return -EOPNOTSUPP;
}
-static inline int bpf_probe_register(struct tracepoint *tp, struct bpf_prog *p)
+static inline int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *p)
{
return -EOPNOTSUPP;
}
-static inline int bpf_probe_unregister(struct tracepoint *tp, struct bpf_prog *p)
+static inline int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_prog *p)
{
return -EOPNOTSUPP;
}
+static inline struct bpf_raw_event_map *bpf_find_raw_tracepoint(const char *name)
+{
+ return NULL;
+}
#endif
enum {
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 39a283c61c51..35db8dd48c4c 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -36,4 +36,9 @@ struct tracepoint {
u32 num_args;
};
+struct bpf_raw_event_map {
+ struct tracepoint *tp;
+ void *bpf_func;
+};
+
#endif
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index f100c63ff19e..6037a2f0108a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1312,7 +1312,7 @@ static int bpf_obj_get(const union bpf_attr *attr)
}
struct bpf_raw_tracepoint {
- struct tracepoint *tp;
+ struct bpf_raw_event_map *btp;
struct bpf_prog *prog;
};
@@ -1321,7 +1321,7 @@ static int bpf_raw_tracepoint_release(struct inode *inode, struct file *filp)
struct bpf_raw_tracepoint *raw_tp = filp->private_data;
if (raw_tp->prog) {
- bpf_probe_unregister(raw_tp->tp, raw_tp->prog);
+ bpf_probe_unregister(raw_tp->btp, raw_tp->prog);
bpf_prog_put(raw_tp->prog);
}
kfree(raw_tp);
@@ -1339,7 +1339,7 @@ static const struct file_operations bpf_raw_tp_fops = {
static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
{
struct bpf_raw_tracepoint *raw_tp;
- struct tracepoint *tp;
+ struct bpf_raw_event_map *btp;
struct bpf_prog *prog;
char tp_name[128];
int tp_fd, err;
@@ -1349,14 +1349,14 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
return -EFAULT;
tp_name[sizeof(tp_name) - 1] = 0;
- tp = kernel_tracepoint_find_by_name(tp_name);
- if (!tp)
+ btp = bpf_find_raw_tracepoint(tp_name);
+ if (!btp)
return -ENOENT;
raw_tp = kmalloc(sizeof(*raw_tp), GFP_USER | __GFP_ZERO);
if (!raw_tp)
return -ENOMEM;
- raw_tp->tp = tp;
+ raw_tp->btp = btp;
prog = bpf_prog_get_type(attr->raw_tracepoint.prog_fd,
BPF_PROG_TYPE_RAW_TRACEPOINT);
@@ -1365,7 +1365,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
goto out_free_tp;
}
- err = bpf_probe_register(raw_tp->tp, prog);
+ err = bpf_probe_register(raw_tp->btp, prog);
if (err)
goto out_put_prog;
@@ -1373,7 +1373,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
tp_fd = anon_inode_getfd("bpf-raw-tracepoint", &bpf_raw_tp_fops, raw_tp,
O_CLOEXEC);
if (tp_fd < 0) {
- bpf_probe_unregister(raw_tp->tp, prog);
+ bpf_probe_unregister(raw_tp->btp, prog);
err = tp_fd;
goto out_put_prog;
}
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index eb58ef156d36..e578b173fe1d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -965,6 +965,19 @@ int perf_event_query_prog_array(struct perf_event *event, void __user *info)
return ret;
}
+extern struct bpf_raw_event_map *__start__bpf_raw_tp[];
+extern struct bpf_raw_event_map *__stop__bpf_raw_tp[];
+
+struct bpf_raw_event_map *bpf_find_raw_tracepoint(const char *name)
+{
+ struct bpf_raw_event_map* const *btp = __start__bpf_raw_tp;
+
+ for (; btp < __stop__bpf_raw_tp; btp++)
+ if (!strcmp((*btp)->tp->name, name))
+ return *btp;
+ return NULL;
+}
+
static __always_inline
void __bpf_trace_run(struct bpf_prog *prog, u64 *args)
{
@@ -1020,10 +1033,9 @@ BPF_TRACE_DEFN_x(10);
BPF_TRACE_DEFN_x(11);
BPF_TRACE_DEFN_x(12);
-static int __bpf_probe_register(struct tracepoint *tp, struct bpf_prog *prog)
+static int __bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog)
{
- unsigned long addr;
- char buf[128];
+ struct tracepoint *tp = btp->tp;
/*
* check that program doesn't access arguments beyond what's
@@ -1032,43 +1044,25 @@ static int __bpf_probe_register(struct tracepoint *tp, struct bpf_prog *prog)
if (prog->aux->max_ctx_offset > tp->num_args * sizeof(u64))
return -EINVAL;
- snprintf(buf, sizeof(buf), "__bpf_trace_%s", tp->name);
- addr = kallsyms_lookup_name(buf);
- if (!addr)
- return -ENOENT;
-
- return tracepoint_probe_register(tp, (void *)addr, prog);
+ return tracepoint_probe_register(tp, (void *)btp->bpf_func, prog);
}
-int bpf_probe_register(struct tracepoint *tp, struct bpf_prog *prog)
+int bpf_probe_register(struct bpf_raw_event_map *btp, struct bpf_prog *prog)
{
int err;
mutex_lock(&bpf_event_mutex);
- err = __bpf_probe_register(tp, prog);
+ err = __bpf_probe_register(btp, prog);
mutex_unlock(&bpf_event_mutex);
return err;
}
-static int __bpf_probe_unregister(struct tracepoint *tp, struct bpf_prog *prog)
-{
- unsigned long addr;
- char buf[128];
-
- snprintf(buf, sizeof(buf), "__bpf_trace_%s", tp->name);
- addr = kallsyms_lookup_name(buf);
- if (!addr)
- return -ENOENT;
-
- return tracepoint_probe_unregister(tp, (void *)addr, prog);
-}
-
-int bpf_probe_unregister(struct tracepoint *tp, struct bpf_prog *prog)
+int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_prog *prog)
{
int err;
mutex_lock(&bpf_event_mutex);
- err = __bpf_probe_unregister(tp, prog);
+ err = tracepoint_probe_unregister(btp->tp, (void *)btp->bpf_func, prog);
mutex_unlock(&bpf_event_mutex);
return err;
}
^ permalink raw reply related
* Re: [PATCH net v2] udp6: set dst cache for a connected sk before udp_v6_send_skb
From: Martin KaFai Lau @ 2018-03-27 19:00 UTC (permalink / raw)
To: Alexey Kodanev; +Cc: netdev, Eric Dumazet, David Miller
In-Reply-To: <7d9ede59-3c29-4b09-eb2e-2650400a7d5c@oracle.com>
On Tue, Mar 27, 2018 at 04:27:30PM +0300, Alexey Kodanev wrote:
> On 26.03.2018 20:02, Martin KaFai Lau wrote:
> > On Mon, Mar 26, 2018 at 05:48:47PM +0300, Alexey Kodanev wrote:
> >> After commit 33c162a980fe ("ipv6: datagram: Update dst cache of a
> >> connected datagram sk during pmtu update"), when the error occurs on
> >> sending datagram in udpv6_sendmsg() due to ICMPV6_PKT_TOOBIG type,
> >> error handler can trigger the following path and call ip6_dst_store():
> >>
> >> udpv6_err()
> >> ip6_sk_update_pmtu()
> >> ip6_datagram_dst_update()
> >> ip6_dst_lookup_flow(), can create a RTF_CACHE clone
> > Instead of ip6_dst_lookup_flow(),
> > you meant the RTF_CACHE route created in ip6_update_pmtu()
> >
>
> Right, or even earlier... I was using vti tunnel and it invokes
> skb_dst_update_pmtu() on this error, then sends ICMPv6_PKT_TOOBIG.
>
> >> ...
> >> ip6_dst_store()
> >>
> >> It can happen before a connected UDP socket invokes ip6_dst_store()
> >> in the end of udpv6_sendmsg(), on destination release, as a result,
> >> the last one changes dst to the old one, preventing getting updated
> >> dst cache on the next udpv6_sendmsg() call.
> >>
> >> This patch moves ip6_dst_store() in udpv6_sendmsg(), so that it is
> >> invoked after ip6_sk_dst_lookup_flow() and before udp_v6_send_skb().
> > After this patch, the above udpv6_err() path could not happen after
> > ip6_sk_dst_lookup_flow() and before the ip6_dst_store() in udpv6_sendmsg()?
> >
>
> May be we could minimize this if save it in ip6_sk_dst_lookup_flow()
> for a connected UDP sockets only if we're not getting it from a cache
> for some reason?
I am just not clear how moving it earlier can completely stop the
described issue. Beside, the next ICMPV6_PKT_TOOBIG will be
received again and eventually rectify sk->sk_dst_cache?
>
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index a8a9195..0204f52 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1115,13 +1115,30 @@ struct dst_entry *ip6_dst_lookup_flow(const struct sock *sk, struct flowi6 *fl6,
> * error code.
> */
> struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
> - const struct in6_addr *final_dst)
> + const struct in6_addr *final_dst,
> + bool connected)
> {
> struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
>
> dst = ip6_sk_dst_check(sk, dst, fl6);
> - if (!dst)
> - dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
> + if (dst)
> + return dst;
> +
> + dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
> +
> + if (connected && !IS_ERR(dst))
> + ip6_dst_store(sk, dst_clone(dst), ...);
Right, I think doing dst_store() only if ip6_sk_dst_check()/
sk_dst_check() returns NULL is a more sensible thing to
do here.
>
> Thanks,
> Alexey
>
> >>
> >> Also, increase refcnt for dst, when passing it to ip6_dst_store()
> >> because after that the dst cache can be released by other calls
> >> to ip6_dst_store() with the same socket.
> >>
> >> Fixes: 33c162a980fe ("ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update")
> >> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> >> ---
> >>
> >> v2: * remove 'release_dst:' label
> >>
> >> * move ip6_dst_store() below MSG_CONFIRM check as
> >> suggested by Eric and add dst_clone()
> >>
> >> * add 'Fixes' commit.
> >>
> >>
> >> net/ipv6/udp.c | 29 +++++++++++------------------
> >> 1 file changed, 11 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> >> index 52e3ea0..4508e5a 100644
> >> --- a/net/ipv6/udp.c
> >> +++ b/net/ipv6/udp.c
> >> @@ -1303,6 +1303,16 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >> goto do_confirm;
> >> back_from_confirm:
> >>
> >> + if (connected)>> + ip6_dst_store(sk, dst_clone(dst),
> >> + ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
> >> + &sk->sk_v6_daddr : NULL,
> >> +#ifdef CONFIG_IPV6_SUBTREES
> >> + ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
> >> + &np->saddr :
> >> +#endif
> >> + NULL);
> >> +
> >> /* Lockless fast path for the non-corking case */
> >> if (!corkreq) {
> >> struct sk_buff *skb;
> >> @@ -1314,7 +1324,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >> err = PTR_ERR(skb);
> >> if (!IS_ERR_OR_NULL(skb))
> >> err = udp_v6_send_skb(skb, &fl6);
> >> - goto release_dst;
> >> + goto out;
> >> }
> >>
> >> lock_sock(sk);
> >> @@ -1348,23 +1358,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >> err = np->recverr ? net_xmit_errno(err) : 0;
> >> release_sock(sk);
> >>
> >> -release_dst:
> >> - if (dst) {
> >> - if (connected) {
> >> - ip6_dst_store(sk, dst,
> >> - ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
> >> - &sk->sk_v6_daddr : NULL,
> >> -#ifdef CONFIG_IPV6_SUBTREES
> >> - ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
> >> - &np->saddr :
> >> -#endif
> >> - NULL);
> >> - } else {
> >> - dst_release(dst);
> >> - }
> >> - dst = NULL;
> >> - }
> >> -
> >> out:
> >> dst_release(dst);
> >> fl6_sock_release(flowlabel);
> >> --
> >> 1.8.3.1
> >>
>
^ permalink raw reply
* Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-27 19:00 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers, Kees Cook, Thomas Gleixner, Ingo Molnar
In-Reply-To: <f071b59d-5d67-0eb6-c5b1-68094b0a5118@fb.com>
On Tue, 27 Mar 2018 11:45:34 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> >> +
> >> + snprintf(buf, sizeof(buf), "__bpf_trace_%s", tp->name);
> >> + addr = kallsyms_lookup_name(buf);
> >> + if (!addr)
> >> + return -ENOENT;
> >> +
> >> + return tracepoint_probe_register(tp, (void *)addr, prog);
> >
> > You are putting in a hell of a lot of trust with kallsyms returning
> > properly. I can see this being very fragile. This is calling a function
> > based on the result of kallsyms. I'm sure the security folks would love
> > this.
> >
> > There's a few things to make this a bit more robust. One is to add a
> > table that points to all __bpf_trace_* functions, and verify that the
> > result from kallsyms is in that table.
> >
> > Honestly, I think this is too much of a short cut and a hack. I know
> > you want to keep it "simple" and save space, but you really should do
> > it the same way ftrace and perf do it. That is, create a section and
> > have all tracepoints create a structure that holds a pointer to the
> > tracepoint and to the bpf probe function. Then you don't even need the
> > kernel_tracepoint_find_by_name(), you just iterate over your table and
> > you get the tracepoint and the bpf function associated to it.
> >
> > Relying on kallsyms to return an address to execute is just way too
> > extreme and fragile for my liking.
>
> Wasting extra 8bytes * number_of_tracepoints just for lack of trust
> in kallsyms doesn't sound like good trade off to me.
> If kallsyms are inaccurate all sorts of things will break:
> kprobes, livepatch, etc.
> I'd rather suggest for ftrace to use kallsyms approach as well
> and reduce memory footprint.
If Linus, Thomas, Peter, Ingo, and the security folks trust kallsyms to
return a valid function pointer from a name, then sure, we can try
going that way.
-- Steve
^ permalink raw reply
* Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-27 19:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers, Kees Cook, Thomas Gleixner, Ingo Molnar
In-Reply-To: <20180327150041.3d86e16e@gandalf.local.home>
On Tue, 27 Mar 2018 15:00:41 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> > Wasting extra 8bytes * number_of_tracepoints just for lack of trust
> > in kallsyms doesn't sound like good trade off to me.
> > If kallsyms are inaccurate all sorts of things will break:
> > kprobes, livepatch, etc.
And if kallsyms breaks, these will break by failing to attach, or some
other benign error. Ftrace uses kallsyms to find functions too, but it
only enables functions based on the result, it doesn't use the result
for anything but to compare it to what it already knows.
This is a first that I know of to trust that kallsyms returns something
that you expect to execute with no other validation. It may be valid,
but it also makes me very nervous too. If others are fine with such an
approach, then OK, we can enter a new chapter of development where we
can use kallsyms to find the functions we want to call and use it.
-- Steve
^ permalink raw reply
* Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-27 19:10 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers, Kees Cook, Thomas Gleixner, Ingo Molnar,
Andrew Morton
In-Reply-To: <20180327150041.3d86e16e@gandalf.local.home>
[ Added Andrew Morton too ]
On Tue, 27 Mar 2018 15:00:41 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 27 Mar 2018 11:45:34 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
> > >> +
> > >> + snprintf(buf, sizeof(buf), "__bpf_trace_%s", tp->name);
> > >> + addr = kallsyms_lookup_name(buf);
> > >> + if (!addr)
> > >> + return -ENOENT;
> > >> +
> > >> + return tracepoint_probe_register(tp, (void *)addr, prog);
> > >
> > > You are putting in a hell of a lot of trust with kallsyms returning
> > > properly. I can see this being very fragile. This is calling a function
> > > based on the result of kallsyms. I'm sure the security folks would love
> > > this.
> > >
> > > There's a few things to make this a bit more robust. One is to add a
> > > table that points to all __bpf_trace_* functions, and verify that the
> > > result from kallsyms is in that table.
> > >
> > > Honestly, I think this is too much of a short cut and a hack. I know
> > > you want to keep it "simple" and save space, but you really should do
> > > it the same way ftrace and perf do it. That is, create a section and
> > > have all tracepoints create a structure that holds a pointer to the
> > > tracepoint and to the bpf probe function. Then you don't even need the
> > > kernel_tracepoint_find_by_name(), you just iterate over your table and
> > > you get the tracepoint and the bpf function associated to it.
> > >
> > > Relying on kallsyms to return an address to execute is just way too
> > > extreme and fragile for my liking.
> >
> > Wasting extra 8bytes * number_of_tracepoints just for lack of trust
> > in kallsyms doesn't sound like good trade off to me.
> > If kallsyms are inaccurate all sorts of things will break:
> > kprobes, livepatch, etc.
> > I'd rather suggest for ftrace to use kallsyms approach as well
> > and reduce memory footprint.
>
> If Linus, Thomas, Peter, Ingo, and the security folks trust kallsyms to
> return a valid function pointer from a name, then sure, we can try
> going that way.
I would like an ack from Linus and/or Andrew before we go further down
this road.
-- Steve
^ permalink raw reply
* Industrial routers with high?performance-price ratio and multiple functions
From: Linda @ 2018-03-27 1:41 UTC (permalink / raw)
To: netdev
Dear Sir or Madam,
If you're on the market for industrial routers, It will be glad to tell you that we can meet all of your requirements .
Our company name is Xiamen Ursalink Technology Co,We are the manufacturer specializing on designing and producing M2M/IoT hardware and solutions.
The features of our products£º
1. High-availability LTE/WCDMA/GSM connection
2. Automated fail-over between Ethernet and cellular (dual SIMs).
3. IPsec, OpenVPN, DMVPN, L2TP, GRE, PPTP for safety communication.
4. Ultra-reliable and secure data transmission via Gigabit Ethernet ports.
5. Fully integrated into Microsoft Auzure IoT eco-system, easily to be build an IoT solution.
6. Python & Ursalink SDK (Python 2.7/C) for secondary development.
7. Free 3-year warranty
8. No additional license fee (All-in-one system)
9. It can work as Modbus Master to send alerts by SMS.
10. It support TCP2COM protocol to integrate with SCADA system.
... ...
Such a high performance-price ratio, with multiple functions, and high security router is your best choice,isn¡¯t it?
To get more information£¬you can click here or visit www.ursalink.com .
More details will be available on receipt of your reply.
^ permalink raw reply
* Re: [PATCH v6 bpf-next 08/11] bpf: introduce BPF_RAW_TRACEPOINT
From: Mathieu Desnoyers @ 2018-03-27 19:10 UTC (permalink / raw)
To: rostedt
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Kees Cook, Thomas Gleixner, Ingo Molnar
In-Reply-To: <20180327150041.3d86e16e@gandalf.local.home>
----- On Mar 27, 2018, at 3:00 PM, rostedt rostedt@goodmis.org wrote:
> On Tue, 27 Mar 2018 11:45:34 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> >> +
>> >> + snprintf(buf, sizeof(buf), "__bpf_trace_%s", tp->name);
>> >> + addr = kallsyms_lookup_name(buf);
>> >> + if (!addr)
>> >> + return -ENOENT;
>> >> +
>> >> + return tracepoint_probe_register(tp, (void *)addr, prog);
>> >
>> > You are putting in a hell of a lot of trust with kallsyms returning
>> > properly. I can see this being very fragile. This is calling a function
>> > based on the result of kallsyms. I'm sure the security folks would love
>> > this.
>> >
>> > There's a few things to make this a bit more robust. One is to add a
>> > table that points to all __bpf_trace_* functions, and verify that the
>> > result from kallsyms is in that table.
>> >
>> > Honestly, I think this is too much of a short cut and a hack. I know
>> > you want to keep it "simple" and save space, but you really should do
>> > it the same way ftrace and perf do it. That is, create a section and
>> > have all tracepoints create a structure that holds a pointer to the
>> > tracepoint and to the bpf probe function. Then you don't even need the
>> > kernel_tracepoint_find_by_name(), you just iterate over your table and
>> > you get the tracepoint and the bpf function associated to it.
>> >
>> > Relying on kallsyms to return an address to execute is just way too
>> > extreme and fragile for my liking.
>>
>> Wasting extra 8bytes * number_of_tracepoints just for lack of trust
>> in kallsyms doesn't sound like good trade off to me.
>> If kallsyms are inaccurate all sorts of things will break:
>> kprobes, livepatch, etc.
>> I'd rather suggest for ftrace to use kallsyms approach as well
>> and reduce memory footprint.
>
> If Linus, Thomas, Peter, Ingo, and the security folks trust kallsyms to
> return a valid function pointer from a name, then sure, we can try
> going that way.
This will crash on ARM Thumb2 kernels. Also, how is this expected to
work on PowerPC ABIv1 without KALLSYMS_ALL ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [net-next PATCH v2 03/10] net: netcp: ethss: make call to gbe_sgmii_config() conditional
From: Murali Karicheri @ 2018-03-27 19:20 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt, mark.rutland, ssantosh, malat, w-kwok2, devicetree,
linux-kernel, linux-arm-kernel, davem, netdev
In-Reply-To: <20180327171343.GN5862@lunn.ch>
On 03/27/2018 01:13 PM, Andrew Lunn wrote:
> On Tue, Mar 27, 2018 at 12:31:42PM -0400, Murali Karicheri wrote:
>> As a preparatory patch to add support for 2u cpsw hardware found on
>> K2G SoC, make call to gbe_sgmii_config() conditional. This is required
>> since 2u uses RGMII interface instead of SGMII and to allow for driver
>> re-use.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> ---
>> drivers/net/ethernet/ti/netcp_ethss.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c
>> index 56dbc0b..1dea891 100644
>> --- a/drivers/net/ethernet/ti/netcp_ethss.c
>> +++ b/drivers/net/ethernet/ti/netcp_ethss.c
>> @@ -2271,7 +2271,8 @@ static int gbe_slave_open(struct gbe_intf *gbe_intf)
>>
>> void (*hndlr)(struct net_device *) = gbe_adjust_link;
>>
>> - gbe_sgmii_config(priv, slave);
>> + if ((priv->ss_version == GBE_SS_VERSION_14) || IS_SS_ID_NU(priv))
>> + gbe_sgmii_config(priv, slave);
>
> Hi Murali
>
> So you have:
>
> #define IS_SS_ID_NU(d) \
> (GBE_IDENT((d)->ss_version) == GBE_SS_ID_NU)
>
>
> Does version 14 have a name? Could you add another IS_SS_ID_XX(d)
> macro for it? That would make these statements more consistent.
>
unfortunately not being the first version :)
Probably we can add
#define IS_SS_ID_VER_14(d) \
(GBE_IDENT((d)->ss_version) == GBE_SS_VERSION_14)
and replace all instances of (priv->ss_version == GBE_SS_VERSION_14) with
if (IS_SS_ID_VER_14(priv)) or equivalent.
> Andrew
>
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf, tracing: unbreak lttng
From: Steven Rostedt @ 2018-03-27 19:35 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, davem, torvalds, peterz, mathieu.desnoyers,
netdev, kernel-team, linux-api
In-Reply-To: <1260578f-4a29-2c72-d81c-3f6e0588bbee@iogearbox.net>
On Tue, 27 Mar 2018 11:39:19 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 03/27/2018 02:07 AM, Steven Rostedt wrote:
> > On Mon, 26 Mar 2018 16:02:20 -0700
> > Alexei Starovoitov <ast@kernel.org> wrote:
> >
> >> for_each_kernel_tracepoint() is used by out-of-tree lttng module
> >> and therefore cannot be changed.
> >
> > This is false and misleading. NACK.
>
> Steven, while I really don't care about this particular function, you wrote
> a few emails ago, quote:
>
> Look, the tracepoint code was written by Mathieu for LTTng, and perf
> and ftrace were able to benefit because of it, as well as your bpf
> code. For this, we agreed to keep this function around for his use,
> as its the only thing he requires. Everyone has been fine with that.
> [...] Having that function for LTTng does not hurt us. And I will NACK
> removing it.
>
> So later saying that "this is false and misleading" is kind of misleading
> by itself. ;-) Anyway, it would probably make sense to add a comment to
> for_each_kernel_tracepoint() that this is used by LTTng so that people
> don't accidentally remove it due to missing in-tree user. I would think
> some sort of clarification/background in a comment or such would have
> avoided the whole confusion and resulting discussion around this in the
> first place.
I agree, a comment should be added. But the function can still be
modified, which is what I meant here by being misleading.
>
> Btw, in networking land, as soon as there is no in-tree user for a particular
> kernel function, it will get ripped out, no matter what. Given this is also
> the typical convention in the kernel, it may have caused some confusion with
> above preference.
This is usually the case with me too. This came from Mathieu doing a
lot of work to help perf and ftrace, but keep this for him to maintain
LTTng. This was the solution to a long drawn out flame war. Yes,
there's a lot of history behind that function, and I agree that we
should comment the history behind it.
>
> Anyway, given v6 is out now, I've tossed the old series from bpf-next tree.
> So I hope we can all move on with some more constructive discussion. :-)
Yes, which we are doing around the kallsyms part. ;-)
-- Steve
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox