* [PATCH 0/4] make function arg and structures as const
From: Bhumika Goyal @ 2017-10-17 16:14 UTC (permalink / raw)
To: julia.lawall, trond.myklebust, anna.schumaker, bfields, jlayton,
davem, linux-nfs, linux-kernel, netdev
Cc: Bhumika Goyal
Make the function argument as const. After thing change, make
the cache_detail structures as const.
Bhumika Goyal (4):
sunrpc: make the function arg as const
NFS: make cache_detail structures const
NFSD: make cache_detail structures const
SUNRPC: make cache_detail structures const
fs/nfs/dns_resolve.c | 2 +-
fs/nfsd/export.c | 4 ++--
fs/nfsd/nfs4idmap.c | 4 ++--
include/linux/sunrpc/cache.h | 2 +-
net/sunrpc/auth_gss/svcauth_gss.c | 4 ++--
net/sunrpc/cache.c | 2 +-
net/sunrpc/svcauth_unix.c | 4 ++--
7 files changed, 11 insertions(+), 11 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: RFC(v2): Audit Kernel Container IDs
From: Casey Schaufler @ 2017-10-17 16:10 UTC (permalink / raw)
To: Simo Sorce, Steve Grubb, linux-audit
Cc: Richard Guy Briggs, mszeredi, Eric W. Biederman, jlayton,
Carlos O'Donell, Linux API, Linux Containers, Linux Kernel,
Eric Paris, David Howells, Al Viro, Andy Lutomirski,
Linux Network Development, Linux FS Devel, cgroups,
Serge E. Hallyn, trondmy
In-Reply-To: <1508254120.6230.34.camel@redhat.com>
On 10/17/2017 8:28 AM, Simo Sorce wrote:
> On Tue, 2017-10-17 at 07:59 -0700, Casey Schaufler wrote:
>> On 10/17/2017 5:31 AM, Simo Sorce wrote:
>>> On Mon, 2017-10-16 at 21:42 -0400, Steve Grubb wrote:
>>>> On Monday, October 16, 2017 8:33:40 PM EDT Richard Guy Briggs
>>>> wrote:
>>>>> There is such a thing, but the kernel doesn't know about it
>>>>> yet. This same situation exists for loginuid and sessionid
>>>>> which
>>>>> are userspace concepts that the kernel tracks for the
>>>>> convenience
>>>>> of userspace. As for its name, I'm not particularly picky, so
>>>>> if
>>>>> you don't like CAP_CONTAINER_* then I'm fine with
>>>>> CAP_AUDIT_CONTAINERID. It really needs to be distinct from
>>>>> CAP_AUDIT_WRITE and CAP_AUDIT_CONTROL since we don't want to
>>>>> give
>>>>> the ability to set a containerID to any process that is able to
>>>>> do
>>>>> audit logging (such as vsftpd) and similarly we don't want to
>>>>> give
>>>>> the orchestrator the ability to control the setup of the audit
>>>>> daemon.
>>>> A long time ago, we were debating what should guard against rouge
>>>> processes from setting the loginuid. Casey argued that the
>>>> ability to
>>>> set the loginuid means they have the ability to control the audit
>>>> trail. That means that it should be guarded by CAP_AUDIT_CONTROL.
>>>> I
>>>> think the same logic applies today.
>>> The difference is that with loginuid you needed to give processes
>>> able
>>> to audit also the ability to change it. You do not want to tie the
>>> ability to change container ids to the ability to audit. You want
>>> to be
>>> able to do audit stuff (within the container) without allowing it
>>> to
>>> change the container id.
>> Without a *kernel* policy on containerIDs you can't say what
>> security policy is being exempted.
> The policy has been basically stated earlier.
No. The expected user space behavior has been stated.
> A way to track a set of processes from a specific point in time
> forward. The name used is "container id", but it could be anything.
Then you want Jose Bollo's PTAGS. It's insane to add yet another
arbitrary ID to the task for a special purpose. Add a general tagging
mechanism instead. We could add a gazillion new id's, each with it's
own capability if we head down this road.
> This marker is mostly used by user space to track process hierarchies
> without races, these processes can be very privileged, and must not be
> allowed to change the marker themselves when granted the current common
> capabilities.
Let's be clear. What happens in user space stays in user space.
The kernel does not give a fig about user space policy. There has
to be a kernel policy involved that a capability can exempt.
> Is this a good enough description ? If not can you clarify your
> expectations ?
The kernel enforces kernel policy. Capabilities provide a mechanism
to mark a process as exempt from some aspect of kernel policy. If
you don't have a kernel policy, you don't get a capability. Clear?
>
>> Without that you can't say what capability is (or isn't)
>> appropriate.
> See if the above is sufficient please.
>
>> You need a reason to have a capability check that makes sense in the
>> context of the kernel security policy.
> I think the proposal had a reason, we may debate on whether that reason
> is good enough.
>
>> Since we don't know what a container is in the kernel,
> Please do not fixate on the word container.
>
>> that's pretty hard. We don't create "fuzzy" capabilities
>> based on the trendy application behavior of the moment. If the
>> behavior is not related it audit, there's no reason for it, and
>> if it is, CAP_AUDIT_CONTROL works just fine. If this doesn't work
>> in your application security model I suggest that is where you
>> need to make changes.
> The authors of the proposal came to the conclusion that kernel
> assistance is needed. It would be nice to discuss the merits of it.
> If you do not understand why the request has been made it would be more
> useful to ask specific questions to understand what and why is the ask.
I understand pretty darn well.
> Pushing back is fine, if you have understood the problem and have valid
> arguments against a kernel level solution (and possibly suggestions for
> a working user space solution), otherwise you are not adding value to
> the discussion.
The presumption is that the request is reasonable. Adding a capability
in support of an undefined behavior is unreasonable. Based on the discussion,
CAP_AUDIT_CONTROL is completely rational. I understand that it would be
difficult to support your application privilege model. I would like to look
into helping out with that, but have too many burning knives in the air
just now.
>
> Simo.
>
^ permalink raw reply
* Re: [PATCH] net: export netdev_txq_to_tc to allow sch_mqprio to compile as module
From: David Miller @ 2017-10-17 16:00 UTC (permalink / raw)
To: henrik
Cc: netdev, edumazet, daniel, dsahern, alexander.h.duyck, willemb,
john.fastabend, me, linux-kernel, haustad, jesus.sanchez-palencia
In-Reply-To: <1508235010-4056-1-git-send-email-haustad@cisco.com>
From: Henrik Austad <henrik@austad.us>
Date: Tue, 17 Oct 2017 12:10:10 +0200
> In commit 32302902ff09 ("mqprio: Reserve last 32 classid values for HW
> traffic classes and misc IDs") sch_mqprio started using netdev_txq_to_tc
> to find the correct tc instead of dev->tc_to_txq[]
>
> However, when mqprio is compiled as a module, it cannot resolve the
> symbol, leading to this error:
>
> ERROR: "netdev_txq_to_tc" [net/sched/sch_mqprio.ko] undefined!
>
> This adds an EXPORT_SYMBOL() since the other user in the kernel
> (netif_set_xps_queue) is also EXPORT_SYMBOL() (and not _GPL) or in a
> sysfs-callback.
>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> Cc: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Henrik Austad <haustad@cisco.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net 3/3] bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
From: Alexei Starovoitov @ 2017-10-17 15:55 UTC (permalink / raw)
To: Daniel Borkmann
Cc: davem, tj, ast, john.fastabend, mark.rutland, richard, sp3485,
netdev, linux-kernel
In-Reply-To: <a11e92b8ba4f5270437caa629307c300bca37976.1508251210.git.daniel@iogearbox.net>
On Tue, Oct 17, 2017 at 04:55:54PM +0200, Daniel Borkmann wrote:
> PCPU_MIN_UNIT_SIZE is an implementation detail of the percpu
> allocator. Given we support __GFP_NOWARN now, lets just let
> the allocation request fail naturally instead. The two call
> sites from BPF mistakenly assumed __GFP_NOWARN would work, so
> no changes needed to their actual __alloc_percpu_gfp() calls
> which use the flag already.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net 2/3] bpf: fix splat for illegal devmap percpu allocation
From: Alexei Starovoitov @ 2017-10-17 15:54 UTC (permalink / raw)
To: Daniel Borkmann
Cc: davem, tj, ast, john.fastabend, mark.rutland, richard, sp3485,
netdev, linux-kernel
In-Reply-To: <dd091b1fec763d74185a156ecd2f972810bdd1ce.1508251210.git.daniel@iogearbox.net>
On Tue, Oct 17, 2017 at 04:55:53PM +0200, Daniel Borkmann wrote:
> It was reported that syzkaller was able to trigger a splat on
> devmap percpu allocation due to illegal/unsupported allocation
> request size passed to __alloc_percpu():
>
> [ 70.094249] illegal size (32776) or align (8) for percpu allocation
> [ 70.094256] ------------[ cut here ]------------
> [ 70.094259] WARNING: CPU: 3 PID: 3451 at mm/percpu.c:1365 pcpu_alloc+0x96/0x630
> [...]
> [ 70.094325] Call Trace:
> [ 70.094328] __alloc_percpu_gfp+0x12/0x20
> [ 70.094330] dev_map_alloc+0x134/0x1e0
> [ 70.094331] SyS_bpf+0x9bc/0x1610
> [ 70.094333] ? selinux_task_setrlimit+0x5a/0x60
> [ 70.094334] ? security_task_setrlimit+0x43/0x60
> [ 70.094336] entry_SYSCALL_64_fastpath+0x1a/0xa5
>
> This was due to too large max_entries for the map such that we
> surpassed the upper limit of PCPU_MIN_UNIT_SIZE. It's fine to
> fail naturally here, so switch to __alloc_percpu_gfp() and pass
> __GFP_NOWARN instead.
>
> Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map")
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Shankara Pailoor <sp3485@columbia.edu>
> Reported-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net 1/3] mm, percpu: add support for __GFP_NOWARN flag
From: Alexei Starovoitov @ 2017-10-17 15:53 UTC (permalink / raw)
To: Daniel Borkmann
Cc: davem, tj, ast, john.fastabend, mark.rutland, richard, sp3485,
netdev, linux-kernel
In-Reply-To: <beeeab23cda20c0b0ee3e4ccb30b91518de6c075.1508251210.git.daniel@iogearbox.net>
On Tue, Oct 17, 2017 at 04:55:52PM +0200, Daniel Borkmann wrote:
> Add an option for pcpu_alloc() to support __GFP_NOWARN flag.
> Currently, we always throw a warning when size or alignment
> is unsupported (and also dump stack on failed allocation
> requests). The warning itself is harmless since we return
> NULL anyway for any failed request, which callers are
> required to handle anyway. However, it becomes harmful when
> panic_on_warn is set.
>
> The rationale for the WARN() in pcpu_alloc() is that it can
> be tracked when larger than supported allocation requests are
> made such that allocations limits can be tweaked if warranted.
> This makes sense for in-kernel users, however, there are users
> of pcpu allocator where allocation size is derived from user
> space requests, e.g. when creating BPF maps. In these cases,
> the requests should fail gracefully without throwing a splat.
>
> The current work-around was to check allocation size against
> the upper limit of PCPU_MIN_UNIT_SIZE from call-sites for
> bailing out prior to a call to pcpu_alloc() in order to
> avoid throwing the WARN(). This is bad in multiple ways since
> PCPU_MIN_UNIT_SIZE is an implementation detail, and having
> the checks on call-sites only complicates the code for no
> good reason. Thus, lets fix it generically by supporting the
> __GFP_NOWARN flag that users can then use with calling the
> __alloc_percpu_gfp() helper instead.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
The approach looks great to me. We've been doing this dance around
allocator warning for long time. It's really not a job of bpf code
to guess into valid parameters of pcpu alloc.
Adding support for __GFP_NOWARN and using it in bpf is much cleaner
fix that avoids layering violations.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic
From: Cong Wang @ 2017-10-17 15:52 UTC (permalink / raw)
To: Chris Mi
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Lucas Bates,
Jiri Pirko, David Miller
In-Reply-To: <VI1PR0501MB21439CB952C540F903969648AB4C0@VI1PR0501MB2143.eurprd05.prod.outlook.com>
On Mon, Oct 16, 2017 at 6:14 PM, Chris Mi <chrism@mellanox.com> wrote:
> I don't think this bug were introduced by above two commits only.
> Actually, this bug were introduced by several commits, at least the following:
> 1. refcnt and bindcnt are not atomic
Nope, it is perfectly okay with non-atomic as long as no parallel,
and without RCU callback they are perfectly serialized by RTNL.
> 2. passing actions using list instead of arrays (I think initially we are using arrays)
We are discussing patch 1/4, this is patch 2/4, so irrelevant.
> 3. using RCU callbacks
This introduces problem 1.
> So instead of blaming the latest commit, it is better to say it is a pre-git error.
You are wrong.
^ permalink raw reply
* [PATCH] [v2] i40e: avoid 64-bit division where possible
From: Arnd Bergmann @ 2017-10-17 15:49 UTC (permalink / raw)
To: Jeff Kirsher
Cc: Arnd Bergmann, Alan Brady, Jacob Keller, Amritha Nambiar,
Mitch Williams, Alexander Duyck, Filip Sadowski, David S. Miller,
Björn Töpel, intel-wired-lan, netdev, linux-kernel
The new bandwidth calculation caused a link error on 32-bit
architectures, like
ERROR: "__aeabi_uldivmod" [drivers/net/ethernet/intel/i40e/i40e.ko] undefined!
The problem is the max_tx_rate calculation that uses 64-bit integers.
This is not really necessary since the numbers are in MBit/s so
they won't be higher than 40000 for the highest support rate, and
are guaranteed to not exceed 2^32 in future generations either.
Another patch from Alan Brady fixed the link error by adding
many calls to do_div(), which makes the code less efficent and
less readable than necessary.
This changes the representation to 'u32' when dealing with MBit/s
and uses div_u64() to convert from u64 numbers in byte/s, reverting
parts of Alan's earlier fix that have become obsolete now.
Fixes: 2027d4deacb1 ("i40e: Add support setting TC max bandwidth rates")
Fixes: 73983b5ae011 ("i40e: fix u64 division usage")
Cc: Alan Brady <alan.brady@intel.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/intel/i40e/i40e.h | 4 +-
drivers/net/ethernet/intel/i40e/i40e_main.c | 70 +++++++++++------------------
2 files changed, 27 insertions(+), 47 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index c3f13120f3ce..c7aa0c982273 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -407,7 +407,7 @@ struct i40e_channel {
u8 enabled_tc;
struct i40e_aqc_vsi_properties_data info;
- u64 max_tx_rate;
+ u32 max_tx_rate; /* in Mbits/s */
/* track this channel belongs to which VSI */
struct i40e_vsi *parent_vsi;
@@ -1101,5 +1101,5 @@ static inline bool i40e_enabled_xdp_vsi(struct i40e_vsi *vsi)
}
int i40e_create_queue_channel(struct i40e_vsi *vsi, struct i40e_channel *ch);
-int i40e_set_bw_limit(struct i40e_vsi *vsi, u16 seid, u64 max_tx_rate);
+int i40e_set_bw_limit(struct i40e_vsi *vsi, u16 seid, u32 max_tx_rate);
#endif /* _I40E_H_ */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 3ceda140170d..57682cc78508 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -5448,17 +5448,16 @@ int i40e_get_link_speed(struct i40e_vsi *vsi)
*
* Helper function to set BW limit for a given VSI
**/
-int i40e_set_bw_limit(struct i40e_vsi *vsi, u16 seid, u64 max_tx_rate)
+int i40e_set_bw_limit(struct i40e_vsi *vsi, u16 seid, u32 max_tx_rate)
{
struct i40e_pf *pf = vsi->back;
- u64 credits = 0;
int speed = 0;
int ret = 0;
speed = i40e_get_link_speed(vsi);
if (max_tx_rate > speed) {
dev_err(&pf->pdev->dev,
- "Invalid max tx rate %llu specified for VSI seid %d.",
+ "Invalid max tx rate %u specified for VSI seid %d.",
max_tx_rate, seid);
return -EINVAL;
}
@@ -5469,13 +5468,12 @@ int i40e_set_bw_limit(struct i40e_vsi *vsi, u16 seid, u64 max_tx_rate)
}
/* Tx rate credits are in values of 50Mbps, 0 is disabled */
- credits = max_tx_rate;
- do_div(credits, I40E_BW_CREDIT_DIVISOR);
- ret = i40e_aq_config_vsi_bw_limit(&pf->hw, seid, credits,
+ ret = i40e_aq_config_vsi_bw_limit(&pf->hw, seid,
+ max_tx_rate / I40E_BW_CREDIT_DIVISOR,
I40E_MAX_BW_INACTIVE_ACCUM, NULL);
if (ret)
dev_err(&pf->pdev->dev,
- "Failed set tx rate (%llu Mbps) for vsi->seid %u, err %s aq_err %s\n",
+ "Failed set tx rate (%u Mbps) for vsi->seid %u, err %s aq_err %s\n",
max_tx_rate, seid, i40e_stat_str(&pf->hw, ret),
i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
return ret;
@@ -6158,17 +6156,13 @@ int i40e_create_queue_channel(struct i40e_vsi *vsi,
/* configure VSI for BW limit */
if (ch->max_tx_rate) {
- u64 credits = ch->max_tx_rate;
-
if (i40e_set_bw_limit(vsi, ch->seid, ch->max_tx_rate))
return -EINVAL;
- do_div(credits, I40E_BW_CREDIT_DIVISOR);
dev_dbg(&pf->pdev->dev,
- "Set tx rate of %llu Mbps (count of 50Mbps %llu) for vsi->seid %u\n",
+ "Set tx rate of %u Mbps (count of 50Mbps %u) for vsi->seid %u\n",
ch->max_tx_rate,
- credits,
- ch->seid);
+ ch->max_tx_rate / I40E_BW_CREDIT_DIVISOR, ch->seid);
}
/* in case of VF, this will be main SRIOV VSI */
@@ -6189,7 +6183,6 @@ int i40e_create_queue_channel(struct i40e_vsi *vsi,
static int i40e_configure_queue_channels(struct i40e_vsi *vsi)
{
struct i40e_channel *ch;
- u64 max_rate = 0;
int ret = 0, i;
/* Create app vsi with the TCs. Main VSI with TC0 is already set up */
@@ -6211,9 +6204,8 @@ static int i40e_configure_queue_channels(struct i40e_vsi *vsi)
/* Bandwidth limit through tc interface is in bytes/s,
* change to Mbit/s
*/
- max_rate = vsi->mqprio_qopt.max_rate[i];
- do_div(max_rate, I40E_BW_MBPS_DIVISOR);
- ch->max_tx_rate = max_rate;
+ ch->max_tx_rate = div_u64(vsi->mqprio_qopt.max_rate[i],
+ I40E_BW_CREDIT_DIVISOR);
list_add_tail(&ch->list, &vsi->ch_list);
@@ -6643,7 +6635,6 @@ static int i40e_validate_mqprio_qopt(struct i40e_vsi *vsi,
struct tc_mqprio_qopt_offload *mqprio_qopt)
{
u64 sum_max_rate = 0;
- u64 max_rate = 0;
int i;
if (mqprio_qopt->qopt.offset[0] != 0 ||
@@ -6658,9 +6649,8 @@ static int i40e_validate_mqprio_qopt(struct i40e_vsi *vsi,
"Invalid min tx rate (greater than 0) specified\n");
return -EINVAL;
}
- max_rate = mqprio_qopt->max_rate[i];
- do_div(max_rate, I40E_BW_MBPS_DIVISOR);
- sum_max_rate += max_rate;
+ sum_max_rate += div_u64(mqprio_qopt->max_rate[i],
+ I40E_BW_CREDIT_DIVISOR);
if (i >= mqprio_qopt->qopt.num_tc - 1)
break;
@@ -6804,18 +6794,14 @@ static int i40e_setup_tc(struct net_device *netdev, void *type_data)
if (pf->flags & I40E_FLAG_TC_MQPRIO) {
if (vsi->mqprio_qopt.max_rate[0]) {
- u64 max_tx_rate = vsi->mqprio_qopt.max_rate[0];
-
- do_div(max_tx_rate, I40E_BW_MBPS_DIVISOR);
+ u32 max_tx_rate = div_u64(vsi->mqprio_qopt.max_rate[0],
+ I40E_BW_CREDIT_DIVISOR);
ret = i40e_set_bw_limit(vsi, vsi->seid, max_tx_rate);
if (!ret) {
- u64 credits = max_tx_rate;
-
- do_div(credits, I40E_BW_CREDIT_DIVISOR);
dev_dbg(&vsi->back->pdev->dev,
- "Set tx rate of %llu Mbps (count of 50Mbps %llu) for vsi->seid %u\n",
+ "Set tx rate of %u Mbps (count of 50Mbps %u) for vsi->seid %u\n",
max_tx_rate,
- credits,
+ max_tx_rate / I40E_BW_CREDIT_DIVISOR,
vsi->seid);
} else {
need_reset = true;
@@ -9024,15 +9010,12 @@ static int i40e_rebuild_channels(struct i40e_vsi *vsi)
return ret;
}
if (ch->max_tx_rate) {
- u64 credits = ch->max_tx_rate;
-
if (i40e_set_bw_limit(vsi, ch->seid,
ch->max_tx_rate))
return -EINVAL;
- do_div(credits, I40E_BW_CREDIT_DIVISOR);
dev_dbg(&vsi->back->pdev->dev,
- "Set tx rate of %llu Mbps (count of 50Mbps %llu) for vsi->seid %u\n",
+ "Set tx rate of %u Mbps (count of 50Mbps %u) for vsi->seid %u\n",
ch->max_tx_rate,
ch->max_tx_rate / I40E_BW_CREDIT_DIVISOR,
ch->seid);
@@ -9314,21 +9297,18 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
}
if (vsi->mqprio_qopt.max_rate[0]) {
- u64 max_tx_rate = vsi->mqprio_qopt.max_rate[0];
- u64 credits = 0;
+ u32 max_tx_rate = div_u64(vsi->mqprio_qopt.max_rate[0],
+ I40E_BW_CREDIT_DIVISOR);
- do_div(max_tx_rate, I40E_BW_MBPS_DIVISOR);
ret = i40e_set_bw_limit(vsi, vsi->seid, max_tx_rate);
- if (ret)
+ if (!ret)
+ dev_dbg(&vsi->back->pdev->dev,
+ "Set tx rate of %u Mbps (count of 50Mbps %u) for vsi->seid %u\n",
+ max_tx_rate,
+ max_tx_rate / I40E_BW_CREDIT_DIVISOR,
+ vsi->seid);
+ else
goto end_unlock;
-
- credits = max_tx_rate;
- do_div(credits, I40E_BW_CREDIT_DIVISOR);
- dev_dbg(&vsi->back->pdev->dev,
- "Set tx rate of %llu Mbps (count of 50Mbps %llu) for vsi->seid %u\n",
- max_tx_rate,
- credits,
- vsi->seid);
}
ret = i40e_rebuild_cloud_filters(vsi, vsi->seid);
--
2.9.0
^ permalink raw reply related
* Re: [PATCH, net-next] i40e: avoid 64-bit division where possible
From: Arnd Bergmann @ 2017-10-17 15:46 UTC (permalink / raw)
To: Jeff Kirsher
Cc: Jacob Keller, Mitch Williams, Alexander Duyck, Amritha Nambiar,
Filip Sadowski, David S. Miller, Björn Töpel,
intel-wired-lan, Networking, Linux Kernel Mailing List
In-Reply-To: <1508253294.2928.2.camel@intel.com>
On Tue, Oct 17, 2017 at 5:14 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Tue, 2017-10-17 at 12:23 +0200, Arnd Bergmann wrote:
>> The new bandwidth calculation causes a link error on 32-bit
>> architectures, like
>>
>> ERROR: "__aeabi_uldivmod" [drivers/net/ethernet/intel/i40e/i40e.ko]
>> undefined!
> Unfortunately your patch does not apply cleanly to my tree. Arnd,
> could you please rebase your patch based my next-queue tree (dev-queue
> branch)? I already have several i40e patches queued up and applied to
> that branch.
I see you already applied a fix from Alan Brady on that branch. I think
his version is sufficient to avoid the build problems, but mine is better.
I've rebased my patch now to revert parts of his fix. Please decide for
yourself whether you want to apply it on top, or are happy enough with
the existing version.
Arnd
^ permalink raw reply
* Re: [PATCH net-next 4/4] bpf: allow access to skb->len from offloads
From: Alexei Starovoitov @ 2017-10-17 15:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, daniel
In-Reply-To: <20171016234056.1964-5-jakub.kicinski@netronome.com>
On Mon, Oct 16, 2017 at 04:40:56PM -0700, Jakub Kicinski wrote:
> Since we are now doing strict checking of what offloads
> may access, make sure skb->len is on that list.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next 3/4] bpf: move knowledge about post-translation offsets out of verifier
From: Alexei Starovoitov @ 2017-10-17 15:45 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, daniel
In-Reply-To: <20171016234056.1964-4-jakub.kicinski@netronome.com>
On Mon, Oct 16, 2017 at 04:40:55PM -0700, Jakub Kicinski wrote:
> Use the fact that verifier ops are now separate from program
> ops to define a separate set of callbacks for verification of
> already translated programs.
>
> Since we expect the analyzer ops to be defined only for
> a small subset of all program types initialize their array
> by hand (don't use linux/bpf_types.h).
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next 2/4] bpf: remove the verifier ops from program structure
From: Alexei Starovoitov @ 2017-10-17 15:45 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, daniel
In-Reply-To: <20171016234056.1964-3-jakub.kicinski@netronome.com>
On Mon, Oct 16, 2017 at 04:40:54PM -0700, Jakub Kicinski wrote:
> Since the verifier ops don't have to be associated with
> the program for its entire lifetime we can move it to
> verifier's struct bpf_verifier_env.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net-next 1/4] bpf: split verifier and program ops
From: Alexei Starovoitov @ 2017-10-17 15:44 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, oss-drivers, daniel
In-Reply-To: <20171016234056.1964-2-jakub.kicinski@netronome.com>
On Mon, Oct 16, 2017 at 04:40:53PM -0700, Jakub Kicinski wrote:
> struct bpf_verifier_ops contains both verifier ops and operations
> used later during program's lifetime (test_run). Split the runtime
> ops into a different structure.
>
> BPF_PROG_TYPE() will now append ## _prog_ops or ## _verifier_ops
> to the names.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: RFC(v2): Audit Kernel Container IDs
From: James Bottomley @ 2017-10-17 15:44 UTC (permalink / raw)
To: Simo Sorce, Casey Schaufler, Steve Grubb,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA
Cc: mszeredi-H+wXaHxf7aLQT0dZR+AlfA, trondmy-7I+n7zu2hftEKMMhf/gKZA,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, Linux API, Linux Containers,
Linux Kernel, David Howells, Carlos O'Donell,
cgroups-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
Andy Lutomirski, Linux Network Development, Linux FS Devel,
Eric Paris, Al Viro
In-Reply-To: <1508254120.6230.34.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, 2017-10-17 at 11:28 -0400, Simo Sorce wrote:
> > Without a *kernel* policy on containerIDs you can't say what
> > security policy is being exempted.
>
> The policy has been basically stated earlier.
>
> A way to track a set of processes from a specific point in time
> forward. The name used is "container id", but it could be anything.
> This marker is mostly used by user space to track process hierarchies
> without races, these processes can be very privileged, and must not
> be allowed to change the marker themselves when granted the current
> common capabilities.
>
> Is this a good enough description ? If not can you clarify your
> expectations ?
I think you mean you want to be able to apply a label to a process
which is inherited across forks. The label should only be susceptible
to modification by something possessing a capability (which one TBD).
The idea is that processes spawned into a container would be labelled
by the container orchestration system. It's unclear what should happen
to processes using nsenter after the fact, but policy for that should
be up to the orchestration system.
The label will be used as a tag for audit information.
I think you were missing label inheritance above.
The security implications are that anything that can change the label
could also hide itself and its doings from the audit system and thus
would be used as a means to evade detection. I actually think this
means the label should be write once (once you've set it, you can't
change it) and orchestration systems should begin as unlabelled
processes allowing them to do arbitrary forks.
For nested containers, I actually think the label should be
hierarchical, so you can add a label for the new nested container but
it still also contains its parents label as well.
James
^ permalink raw reply
* Re: [PATCH net-next] tcp: Check daddr_cache before use in tracepoint
From: David Ahern @ 2017-10-17 15:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, xiyou.wangcong
In-Reply-To: <1508211908.31614.90.camel@edumazet-glaptop3.roam.corp.google.com>
On 10/16/17 9:45 PM, Eric Dumazet wrote:
> IPV6 TCP uses sk->sk_v6_daddr and sk->->sk_v6_rcv_saddr
I moved v2 to those.
> So I would rather remove the need to fetch np = inet6_sk(sk) in the
> first place, and look at sk->sk_family instead.
I'll spin a v3 with that change.
^ permalink raw reply
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config()
From: Xin Long @ 2017-10-17 15:31 UTC (permalink / raw)
To: Eric Dumazet
Cc: Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner, netdev,
Wei Wang, Eric Dumazet, linux-sctp
In-Reply-To: <1508247956.31614.103.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> SCTP experts.
>
> syszkaller reported a few crashes in sctp_packet_config() with invalid
> access to a deleted dst.
>
> The rcu_read_lock() in sctp_packet_config() is suspect.
>
> It does not protect anything at the moment.
>
> If we expect tp->dst to be manipulated/changed by another cpu/thread,
> then we need proper rcu protection.
>
> Following patch to show what would be a minimal change (but obviously
> bigger changes are needed, like sctp_transport_pmtu_check() and
> sctp_transport_dst_check(), and proper sparse annotations)
will check all places accessing tp->dst in sctp.
>
>
> BTW, sparse throws a lot of errors, any volunteer to clean this mess ?
will do it.
Thanks for reporting this.
>
> make C=2 M=net/sctp
>
> Thanks.
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 4a865cd06d76cd5b2aa417de618da3203f7b53e4..d7f320f5acc271189ec9474795b6ececed7ad2b9 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -86,6 +86,7 @@ void sctp_packet_config(struct sctp_packet *packet, __u32 vtag,
> {
> struct sctp_transport *tp = packet->transport;
> struct sctp_association *asoc = tp->asoc;
> + struct dst_entry *dst;
> struct sock *sk;
>
> pr_debug("%s: packet:%p vtag:0x%x\n", __func__, packet, vtag);
> @@ -121,17 +122,15 @@ void sctp_packet_config(struct sctp_packet *packet, __u32 vtag,
> sctp_packet_append_chunk(packet, chunk);
> }
>
> - if (!tp->dst)
> - return;
> -
> /* set packet max_size with gso_max_size if gso is enabled*/
> rcu_read_lock();
> - if (__sk_dst_get(sk) != tp->dst) {
> - dst_hold(tp->dst);
> - sk_setup_caps(sk, tp->dst);
> + dst = rcu_dereference(tp->dst);
> + if (dst) {
> + if (__sk_dst_get(sk) != dst && dst_hold_safe(dst))
> + sk_setup_caps(sk, dst);
> + packet->max_size = sk_can_gso(sk) ? dst->dev->gso_max_size
> + : asoc->pathmtu;
> }
> - packet->max_size = sk_can_gso(sk) ? tp->dst->dev->gso_max_size
> - : asoc->pathmtu;
> rcu_read_unlock();
> }
>
>
>
^ permalink raw reply
* Re: RFC(v2): Audit Kernel Container IDs
From: Simo Sorce @ 2017-10-17 15:28 UTC (permalink / raw)
To: Casey Schaufler, Steve Grubb, linux-audit-H+wXaHxf7aLQT0dZR+AlfA
Cc: mszeredi-H+wXaHxf7aLQT0dZR+AlfA, trondmy-7I+n7zu2hftEKMMhf/gKZA,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, Linux API, Linux Containers,
Linux Kernel, David Howells, Carlos O'Donell,
cgroups-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
Andy Lutomirski, Linux Network Development, Linux FS Devel,
Eric Paris, Al Viro
In-Reply-To: <a07968f6-fef1-f49d-01f1-6c660c0ada20-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org>
On Tue, 2017-10-17 at 07:59 -0700, Casey Schaufler wrote:
> On 10/17/2017 5:31 AM, Simo Sorce wrote:
> > On Mon, 2017-10-16 at 21:42 -0400, Steve Grubb wrote:
> > > On Monday, October 16, 2017 8:33:40 PM EDT Richard Guy Briggs
> > > wrote:
> > > > There is such a thing, but the kernel doesn't know about it
> > > > yet. This same situation exists for loginuid and sessionid
> > > > which
> > > > are userspace concepts that the kernel tracks for the
> > > > convenience
> > > > of userspace. As for its name, I'm not particularly picky, so
> > > > if
> > > > you don't like CAP_CONTAINER_* then I'm fine with
> > > > CAP_AUDIT_CONTAINERID. It really needs to be distinct from
> > > > CAP_AUDIT_WRITE and CAP_AUDIT_CONTROL since we don't want to
> > > > give
> > > > the ability to set a containerID to any process that is able to
> > > > do
> > > > audit logging (such as vsftpd) and similarly we don't want to
> > > > give
> > > > the orchestrator the ability to control the setup of the audit
> > > > daemon.
> > >
> > > A long time ago, we were debating what should guard against rouge
> > > processes from setting the loginuid. Casey argued that the
> > > ability to
> > > set the loginuid means they have the ability to control the audit
> > > trail. That means that it should be guarded by CAP_AUDIT_CONTROL.
> > > I
> > > think the same logic applies today.
> >
> > The difference is that with loginuid you needed to give processes
> > able
> > to audit also the ability to change it. You do not want to tie the
> > ability to change container ids to the ability to audit. You want
> > to be
> > able to do audit stuff (within the container) without allowing it
> > to
> > change the container id.
>
> Without a *kernel* policy on containerIDs you can't say what
> security policy is being exempted.
The policy has been basically stated earlier.
A way to track a set of processes from a specific point in time
forward. The name used is "container id", but it could be anything.
This marker is mostly used by user space to track process hierarchies
without races, these processes can be very privileged, and must not be
allowed to change the marker themselves when granted the current common
capabilities.
Is this a good enough description ? If not can you clarify your
expectations ?
> Without that you can't say what capability is (or isn't)
> appropriate.
See if the above is sufficient please.
> You need a reason to have a capability check that makes sense in the
> context of the kernel security policy.
I think the proposal had a reason, we may debate on whether that reason
is good enough.
> Since we don't know what a container is in the kernel,
Please do not fixate on the word container.
> that's pretty hard. We don't create "fuzzy" capabilities
> based on the trendy application behavior of the moment. If the
> behavior is not related it audit, there's no reason for it, and
> if it is, CAP_AUDIT_CONTROL works just fine. If this doesn't work
> in your application security model I suggest that is where you
> need to make changes.
The authors of the proposal came to the conclusion that kernel
assistance is needed. It would be nice to discuss the merits of it.
If you do not understand why the request has been made it would be more
useful to ask specific questions to understand what and why is the ask.
Pushing back is fine, if you have understood the problem and have valid
arguments against a kernel level solution (and possibly suggestions for
a working user space solution), otherwise you are not adding value to
the discussion.
Simo.
--
Simo Sorce
Sr. Principal Software Engineer
Red Hat, Inc
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* [PATCH net] sctp: do not peel off an assoc from one netns to another one
From: Xin Long @ 2017-10-17 15:26 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, chunwang, syzkaller
Now when peeling off an association to the sock in another netns, all
transports in this assoc are not to be rehashed and keep use the old
key in hashtable.
As a transport uses sk->net as the hash key to insert into hashtable,
it would miss removing these transports from hashtable due to the new
netns when closing the sock and all transports are being freeed, then
later an use-after-free issue could be caused when looking up an asoc
and dereferencing those transports.
This is a very old issue since very beginning, ChunYu found it with
syzkaller fuzz testing with this series:
socket$inet6_sctp()
bind$inet6()
sendto$inet6()
unshare(0x40000000)
getsockopt$inet_sctp6_SCTP_GET_ASSOC_ID_LIST()
getsockopt$inet_sctp6_SCTP_SOCKOPT_PEELOFF()
This patch is to block this call when peeling one assoc off from one
netns to another one, so that the netns of all transport would not
go out-sync with the key in hashtable.
Note that this patch didn't fix it by rehashing transports, as it's
difficult to handle the situation when the tuple is already in use
in the new netns. Besides, no one would like to peel off one assoc
to another netns, considering ipaddrs, ifaces, etc. are usually
different.
Reported-by: ChunYu Wang <chunwang@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/socket.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index d4730ad..17841ab 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4906,6 +4906,10 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
struct socket *sock;
int err = 0;
+ /* Do not peel off from one netns to another one. */
+ if (!net_eq(current->nsproxy->net_ns, sock_net(sk)))
+ return -EINVAL;
+
if (!asoc)
return -EINVAL;
--
2.1.0
^ permalink raw reply related
* Re: [PATCH, net-next] i40e: avoid 64-bit division where possible
From: Jeff Kirsher @ 2017-10-17 15:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jacob Keller, Mitch Williams, Alexander Duyck, Amritha Nambiar,
Filip Sadowski, David S. Miller, Björn Töpel,
intel-wired-lan, netdev, linux-kernel
In-Reply-To: <20171017102351.492492-1-arnd@arndb.de>
[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]
On Tue, 2017-10-17 at 12:23 +0200, Arnd Bergmann wrote:
> The new bandwidth calculation causes a link error on 32-bit
> architectures, like
>
> ERROR: "__aeabi_uldivmod" [drivers/net/ethernet/intel/i40e/i40e.ko]
> undefined!
>
> The problem is the max_tx_rate calculation that uses 64-bit integers.
> This is not really necessary since the numbers are in MBit/s so
> they won't be higher than 40000 for the highest support rate, and
> are guaranteed to not exceed 2^32 in future generations either.
>
> This changes the representation to 'u32' when dealing with MBit/s
> and uses div_u64() to convert from u64 numbers in byte/s.
>
> Fixes: 2027d4deacb1 ("i40e: Add support setting TC max bandwidth
> rates")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 4 ++--
> drivers/net/ethernet/intel/i40e/i40e_main.c | 27 ++++++++++++++-----
> --------
> 2 files changed, 16 insertions(+), 15 deletions(-)
Unfortunately your patch does not apply cleanly to my tree. Arnd,
could you please rebase your patch based my next-queue tree (dev-queue
branch)? I already have several i40e patches queued up and applied to
that branch.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net 0/3] Fix for BPF devmap percpu allocation splat
From: Daniel Borkmann @ 2017-10-17 15:11 UTC (permalink / raw)
To: David Laight, davem@davemloft.net
Cc: tj@kernel.org, ast@kernel.org, john.fastabend@gmail.com,
mark.rutland@arm.com, richard@nod.at, sp3485@columbia.edu,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD009980D@AcuExch.aculab.com>
On 10/17/2017 05:03 PM, David Laight wrote:
> From: Daniel Borkmann
>> Sent: 17 October 2017 15:56
>>
>> The set fixes a splat in devmap percpu allocation when we alloc
>> the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
>> patch 1 is rather small, so if this could be routed via -net, for
>> example, with Tejun's Ack that would be good. Patch 3 gets rid of
>> remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
>> internals and should not be used.
>
> Does it make sense to allow the user program to try to allocate ever
> smaller very large maps until it finds one that succeeds - thus
> using up all the percpu space?
>
> Or is this a 'root only' 'shoot self in foot' job?
It's root only although John still has a pending fix to be flushed
out for -net first in the next days to actually enforce that cap
(devmap is not in an official kernel yet at this point, so all good),
but apart from this, all map allocs in general are accounted for
as well.
Thanks,
Daniel
^ permalink raw reply
* RE: [PATCH net 0/3] Fix for BPF devmap percpu allocation splat
From: David Laight @ 2017-10-17 15:03 UTC (permalink / raw)
To: 'Daniel Borkmann', davem@davemloft.net
Cc: tj@kernel.org, ast@kernel.org, john.fastabend@gmail.com,
mark.rutland@arm.com, richard@nod.at, sp3485@columbia.edu,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <cover.1508251210.git.daniel@iogearbox.net>
From: Daniel Borkmann
> Sent: 17 October 2017 15:56
>
> The set fixes a splat in devmap percpu allocation when we alloc
> the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
> patch 1 is rather small, so if this could be routed via -net, for
> example, with Tejun's Ack that would be good. Patch 3 gets rid of
> remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
> internals and should not be used.
Does it make sense to allow the user program to try to allocate ever
smaller very large maps until it finds one that succeeds - thus
using up all the percpu space?
Or is this a 'root only' 'shoot self in foot' job?
David
^ permalink raw reply
* [PATCH][next] mqprio: fix potential null pointer dereference on opt
From: Colin King @ 2017-10-17 15:01 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The pointer opt has a null check however before for this check opt is
dereferenced when len is initialized, hence we potentially have a null
pointer deference on opt. Avoid this by checking for a null opt before
dereferencing it.
Detected by CoverityScan, CID#1458234 ("Dereference before null check")
Fixes: 4e8b86c06269 ("mqprio: Introduce new hardware offload mode and shaper in mqprio")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/sched/sch_mqprio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index f1ae9be83934..37d5fd9b617c 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -142,7 +142,7 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
struct nlattr *tb[TCA_MQPRIO_MAX + 1];
struct nlattr *attr;
int rem;
- int len = nla_len(opt) - NLA_ALIGN(sizeof(*qopt));
+ int len;
BUILD_BUG_ON(TC_MAX_QUEUE != TC_QOPT_MAX_QUEUE);
BUILD_BUG_ON(TC_BITMASK != TC_QOPT_BITMASK);
@@ -160,6 +160,7 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
if (mqprio_parse_opt(dev, qopt))
return -EINVAL;
+ len = nla_len(opt) - NLA_ALIGN(sizeof(*qopt));
if (len > 0) {
err = parse_attr(tb, TCA_MQPRIO_MAX, opt, mqprio_policy,
sizeof(*qopt));
--
2.14.1
^ permalink raw reply related
* Re: RFC(v2): Audit Kernel Container IDs
From: Casey Schaufler @ 2017-10-17 14:59 UTC (permalink / raw)
To: Simo Sorce, Steve Grubb, linux-audit
Cc: Richard Guy Briggs, mszeredi, Eric W. Biederman, jlayton,
Carlos O'Donell, Linux API, Linux Containers, Linux Kernel,
Eric Paris, David Howells, Al Viro, Andy Lutomirski,
Linux Network Development, Linux FS Devel, cgroups,
Serge E. Hallyn, trondmy
In-Reply-To: <1508243469.6230.24.camel@redhat.com>
On 10/17/2017 5:31 AM, Simo Sorce wrote:
> On Mon, 2017-10-16 at 21:42 -0400, Steve Grubb wrote:
>> On Monday, October 16, 2017 8:33:40 PM EDT Richard Guy Briggs wrote:
>>> There is such a thing, but the kernel doesn't know about it
>>> yet. This same situation exists for loginuid and sessionid which
>>> are userspace concepts that the kernel tracks for the convenience
>>> of userspace. As for its name, I'm not particularly picky, so if
>>> you don't like CAP_CONTAINER_* then I'm fine with
>>> CAP_AUDIT_CONTAINERID. It really needs to be distinct from
>>> CAP_AUDIT_WRITE and CAP_AUDIT_CONTROL since we don't want to give
>>> the ability to set a containerID to any process that is able to do
>>> audit logging (such as vsftpd) and similarly we don't want to give
>>> the orchestrator the ability to control the setup of the audit
>>> daemon.
>> A long time ago, we were debating what should guard against rouge
>> processes from setting the loginuid. Casey argued that the ability to
>> set the loginuid means they have the ability to control the audit
>> trail. That means that it should be guarded by CAP_AUDIT_CONTROL. I
>> think the same logic applies today.
> The difference is that with loginuid you needed to give processes able
> to audit also the ability to change it. You do not want to tie the
> ability to change container ids to the ability to audit. You want to be
> able to do audit stuff (within the container) without allowing it to
> change the container id.
Without a *kernel* policy on containerIDs you can't say what
security policy is being exempted. Without that you can't say what
capability is (or isn't) appropriate. You need a reason to have
a capability check that makes sense in the context of the kernel
security policy. Since we don't know what a container is in the
kernel, that's pretty hard. We don't create "fuzzy" capabilities
based on the trendy application behavior of the moment. If the
behavior is not related it audit, there's no reason for it, and
if it is, CAP_AUDIT_CONTROL works just fine. If this doesn't work
in your application security model I suggest that is where you
need to make changes.
> Of course if we made container id a write-once property maybe there is
> no need for controls at all, but I'm pretty sure there will be
> situations where write-once may not be usable in practice.
>
>> The ability to arbitrarily set a container ID means the process has
>> the ability to indirectly control the audit trail.
> The container Id can be used also for authorization purposes (by other
> processes on the host), not just audit, I think this is why a separate
> control has been proposed.
>
> Simo.
>
^ permalink raw reply
* [PATCH net 3/3] bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
From: Daniel Borkmann @ 2017-10-17 14:55 UTC (permalink / raw)
To: davem
Cc: tj, ast, john.fastabend, mark.rutland, richard, sp3485, netdev,
linux-kernel, Daniel Borkmann
In-Reply-To: <cover.1508251210.git.daniel@iogearbox.net>
PCPU_MIN_UNIT_SIZE is an implementation detail of the percpu
allocator. Given we support __GFP_NOWARN now, lets just let
the allocation request fail naturally instead. The two call
sites from BPF mistakenly assumed __GFP_NOWARN would work, so
no changes needed to their actual __alloc_percpu_gfp() calls
which use the flag already.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/arraymap.c | 2 +-
kernel/bpf/hashtab.c | 4 ----
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 98c0f00..e263673 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -98,7 +98,7 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr)
array_size += (u64) attr->max_entries * elem_size * num_possible_cpus();
if (array_size >= U32_MAX - PAGE_SIZE ||
- elem_size > PCPU_MIN_UNIT_SIZE || bpf_array_alloc_percpu(array)) {
+ bpf_array_alloc_percpu(array)) {
bpf_map_area_free(array);
return ERR_PTR(-ENOMEM);
}
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 431126f..6533f08 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -317,10 +317,6 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)
*/
goto free_htab;
- if (percpu && round_up(htab->map.value_size, 8) > PCPU_MIN_UNIT_SIZE)
- /* make sure the size for pcpu_alloc() is reasonable */
- goto free_htab;
-
htab->elem_size = sizeof(struct htab_elem) +
round_up(htab->map.key_size, 8);
if (percpu)
--
1.9.3
^ permalink raw reply related
* [PATCH net 2/3] bpf: fix splat for illegal devmap percpu allocation
From: Daniel Borkmann @ 2017-10-17 14:55 UTC (permalink / raw)
To: davem
Cc: tj, ast, john.fastabend, mark.rutland, richard, sp3485, netdev,
linux-kernel, Daniel Borkmann
In-Reply-To: <cover.1508251210.git.daniel@iogearbox.net>
It was reported that syzkaller was able to trigger a splat on
devmap percpu allocation due to illegal/unsupported allocation
request size passed to __alloc_percpu():
[ 70.094249] illegal size (32776) or align (8) for percpu allocation
[ 70.094256] ------------[ cut here ]------------
[ 70.094259] WARNING: CPU: 3 PID: 3451 at mm/percpu.c:1365 pcpu_alloc+0x96/0x630
[...]
[ 70.094325] Call Trace:
[ 70.094328] __alloc_percpu_gfp+0x12/0x20
[ 70.094330] dev_map_alloc+0x134/0x1e0
[ 70.094331] SyS_bpf+0x9bc/0x1610
[ 70.094333] ? selinux_task_setrlimit+0x5a/0x60
[ 70.094334] ? security_task_setrlimit+0x43/0x60
[ 70.094336] entry_SYSCALL_64_fastpath+0x1a/0xa5
This was due to too large max_entries for the map such that we
surpassed the upper limit of PCPU_MIN_UNIT_SIZE. It's fine to
fail naturally here, so switch to __alloc_percpu_gfp() and pass
__GFP_NOWARN instead.
Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map")
Reported-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Shankara Pailoor <sp3485@columbia.edu>
Reported-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/devmap.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index e093d9a..920428d 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -111,8 +111,9 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
err = -ENOMEM;
/* A per cpu bitfield with a bit per possible net device */
- dtab->flush_needed = __alloc_percpu(dev_map_bitmap_size(attr),
- __alignof__(unsigned long));
+ dtab->flush_needed = __alloc_percpu_gfp(dev_map_bitmap_size(attr),
+ __alignof__(unsigned long),
+ GFP_KERNEL | __GFP_NOWARN);
if (!dtab->flush_needed)
goto free_dtab;
--
1.9.3
^ permalink raw reply related
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