* Re: [PATCH 1/1] mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
From: David Miller @ 2019-06-25 21:08 UTC (permalink / raw)
To: saeedm; +Cc: jes.sorensen, kernel-team, jsorensen, netdev
In-Reply-To: <91260adb2227e477647afda66fdff9d9a9f52c60.camel@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Tue, 25 Jun 2019 21:01:58 +0000
> BTW is there a way to clear up "Awaiting Upstream" clutter [1] for mlx5
> patches that are already pulled ?
>
> [1]
> https://patchwork.ozlabs.org/project/netdev/list/?series=&submitter=&state=8&q=mlx5&archive=&delegate=
I don't understand what the problem is. Everything there is in the
appropriate state.
When something hits netdev that doesn't go directly to my tree, that's
the approprate state forever.
^ permalink raw reply
* Re: [PATCH net-next] Revert "net: ena: ethtool: add extra properties retrieval via get_priv_flags"
From: David Miller @ 2019-06-25 21:09 UTC (permalink / raw)
To: jakub.kicinski
Cc: netdev, oss-drivers, nafea, dwmw2, sameehj, zorik, saeedb,
netanel
In-Reply-To: <20190625165956.19278-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 25 Jun 2019 09:59:56 -0700
> This reverts commit 315c28d2b714 ("net: ena: ethtool: add extra properties retrieval via get_priv_flags").
>
> As discussed at netconf and on the mailing list we can't allow
> for the the abuse of private flags for exposing arbitrary device
> labels.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied, thanks Jakub.
^ permalink raw reply
* Re: [PATCH 0/1] Fix broken build of mlx5
From: Saeed Mahameed @ 2019-06-25 21:11 UTC (permalink / raw)
To: jes.sorensen@gmail.com
Cc: kernel-team@fb.com, jsorensen@fb.com, netdev@vger.kernel.org
In-Reply-To: <74cb713f-ad8c-7f86-c611-9dd4265f1c9b@gmail.com>
On Tue, 2019-06-25 at 14:01 -0400, Jes Sorensen wrote:
> On 6/25/19 1:54 PM, Saeed Mahameed wrote:
> > On Tue, 2019-06-25 at 11:27 -0400, Jes Sorensen wrote:
> > > From: Jes Sorensen <jsorensen@fb.com>
> > >
> > > This fixes an obvious build error that could have been caught by
> > > simply building the code before pushing out the patch.
> > >
> >
> > Hi Jes,
> >
> > Just tested again, as I have tested before submitting the blamed
> > patch,
> > and as we test on every single new patch in our build automation.
> >
> > both combinations CONFIG_MLX5_EN_RXNFC=y/n work on latest net-next,
> > what am i missing ?
>
> Linus' tree:
>
> [jes@xpeas linux.git]$ grep mlx5e_get_rxnfc
> drivers/net/ethernet/mellanox/mlx5/core/*.c
> drivers/net/ethernet/mellanox/mlx5/core/en/*.h
> drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c:static int
> mlx5e_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
> u32
> *rule_locs)
> drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c: .get_rxnfc
> = mlx5e_get_rxnfc,
> drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c:int
> mlx5e_get_rxnfc(struct net_device *dev,
> drivers/net/ethernet/mellanox/mlx5/core/en/fs.h:int
> mlx5e_get_rxnfc(struct net_device *dev,
>
> static vs non static functions, with a prototype that is non static.
But only one prototype can be selected and it is according
CONFIG_MLX5_EN_RXNFC
when CONFIG_MLX5_EN_RXNFC=n then the static prototype will be selected
and the other one will be compiled out.
when CONFIG_MLX5_EN_RXNFC=y the non static prototype will be selected
and the static one will be compiled out.
So no issue here.
>
> Jes
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-25 21:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: Daniel Santos, René van Dorst, sean.wang, f.fainelli, davem,
matthias.bgg, vivien.didelot, frank-w, netdev, linux-mediatek,
linux-mips
In-Reply-To: <20190625190246.GA27733@lunn.ch>
On Tue, Jun 25, 2019 at 09:02:46PM +0200, Andrew Lunn wrote:
> > But will there still be a mechanism to ignore link partner's advertising
> > and force these parameters?
>
> From man 1 ethtool:
>
> -a --show-pause
> Queries the specified Ethernet device for pause parameter information.
>
> -A --pause
> Changes the pause parameters of the specified Ethernet device.
>
> autoneg on|off
> Specifies whether pause autonegotiation should be enabled.
>
> rx on|off
> Specifies whether RX pause should be enabled.
>
> tx on|off
> Specifies whether TX pause should be enabled.
>
> You need to check the driver to see if it actually implements this
> ethtool call, but that is how it should be configured.
Note that phylink provides this call, and provided mac_config() is
correctly implemented, will result in the pause mode parameters in
the MAC being correctly set.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [for-next V2 10/10] RDMA/core: Provide RDMA DIM support for ULPs
From: Sagi Grimberg @ 2019-06-25 21:14 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman, Max Gurtovoy
In-Reply-To: <20190625205701.17849-11-saeedm@mellanox.com>
> +static int ib_poll_dim_handler(struct irq_poll *iop, int budget)
> +{
> + struct ib_cq *cq = container_of(iop, struct ib_cq, iop);
> + struct dim *dim = cq->dim;
> + int completed;
> +
> + completed = __ib_process_cq(cq, budget, cq->wc, IB_POLL_BATCH);
> + if (completed < budget) {
> + irq_poll_complete(&cq->iop);
> + if (ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
> + irq_poll_sched(&cq->iop);
> + }
> +
> + rdma_dim(dim, completed);
Why duplicate the entire thing for a one-liner?
> +
> + return completed;
> +}
> +
> static void ib_cq_completion_softirq(struct ib_cq *cq, void *private)
> {
> irq_poll_sched(&cq->iop);
> @@ -105,14 +157,18 @@ static void ib_cq_completion_softirq(struct ib_cq *cq, void *private)
>
> static void ib_cq_poll_work(struct work_struct *work)
> {
> - struct ib_cq *cq = container_of(work, struct ib_cq, work);
> + struct ib_cq *cq = container_of(work, struct ib_cq,
> + work);
Why was that changed?
> int completed;
>
> completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE, cq->wc,
> IB_POLL_BATCH);
> +
newline?
> if (completed >= IB_POLL_BUDGET_WORKQUEUE ||
> ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
> queue_work(cq->comp_wq, &cq->work);
> + else if (cq->dim)
> + rdma_dim(cq->dim, completed);
> }
>
> static void ib_cq_completion_workqueue(struct ib_cq *cq, void *private)
> @@ -166,6 +222,8 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
> rdma_restrack_set_task(&cq->res, caller);
> rdma_restrack_kadd(&cq->res);
>
> + rdma_dim_init(cq);
> +
> switch (cq->poll_ctx) {
> case IB_POLL_DIRECT:
> cq->comp_handler = ib_cq_completion_direct;
> @@ -173,7 +231,13 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
> case IB_POLL_SOFTIRQ:
> cq->comp_handler = ib_cq_completion_softirq;
>
> - irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ, ib_poll_handler);
> + if (cq->dim) {
> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
> + ib_poll_dim_handler);
> + } else
> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
> + ib_poll_handler);
> +
> ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
> break;
> case IB_POLL_WORKQUEUE:
> @@ -226,6 +290,9 @@ void ib_free_cq_user(struct ib_cq *cq, struct ib_udata *udata)
> WARN_ON_ONCE(1);
> }
>
> + if (cq->dim)
> + cancel_work_sync(&cq->dim->work);
> + kfree(cq->dim);
> kfree(cq->wc);
> rdma_restrack_del(&cq->res);
> ret = cq->device->ops.destroy_cq(cq, udata);
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index abac70ad5c7c..b1b45dbe24a5 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -6305,6 +6305,8 @@ static int mlx5_ib_stage_caps_init(struct mlx5_ib_dev *dev)
> MLX5_CAP_GEN(dev->mdev, disable_local_lb_mc)))
> mutex_init(&dev->lb.mutex);
>
> + dev->ib_dev.use_cq_dim = true;
> +
Please don't. This is a bad choice to opt it in by default.
^ permalink raw reply
* Re: [for-next V2 09/10] RDMA/nldev: Added configuration of RDMA dynamic interrupt moderation to netlink
From: Sagi Grimberg @ 2019-06-25 21:15 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman
In-Reply-To: <20190625205701.17849-10-saeedm@mellanox.com>
On 6/25/19 1:57 PM, Saeed Mahameed wrote:
> From: Yamin Friedman <yaminf@mellanox.com>
>
> Added parameter in ib_device for enabling dynamic interrupt moderation so
> that it can be configured in userspace using rdma tool.
>
> In order to set dim for an ib device the command is:
> rdma dev set [DEV] dim [on|off]
> Please set on/off.
Is "dim" what you want to expose to the user? maybe
"adaptive-moderation" is more friendly?
^ permalink raw reply
* Re: [PATCH bpf-next 0/4] sys_bpf() access control via /dev/bpf
From: Stanislav Fomichev @ 2019-06-25 21:19 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Song Liu, netdev@vger.kernel.org, bpf@vger.kernel.org,
ast@kernel.org, daniel@iogearbox.net, Kernel Team
In-Reply-To: <59e56064-354c-d6b9-101a-c698976e6723@fb.com>
On 06/25, Alexei Starovoitov wrote:
> On 6/25/19 1:51 PM, Stanislav Fomichev wrote:
> > On 06/25, Song Liu wrote:
> >> Currently, most access to sys_bpf() is limited to root. However, there are
> >> use cases that would benefit from non-privileged use of sys_bpf(), e.g.
> >> systemd.
> >>
> >> This set introduces a new model to control the access to sys_bpf(). A
> >> special device, /dev/bpf, is introduced to manage access to sys_bpf().
> >> Users with access to open /dev/bpf will be able to access most of
> >> sys_bpf() features. The use can get access to sys_bpf() by opening /dev/bpf
> >> and use ioctl to get/put permission.
> >>
> >> The permission to access sys_bpf() is marked by bit TASK_BPF_FLAG_PERMITTED
> >> in task_struct. During fork(), child will not inherit this bit.
> > 2c: if we are going to have an fd, I'd vote for a proper fd based access
> > checks instead of a per-task flag, so we can do:
> > ioctl(fd, BPF_MAP_CREATE, uattr, sizeof(uattr))
> >
> > (and pass this fd around)
> >
> > I do understand that it breaks current assumptions that libbpf has,
> > but maybe we can extend _xattr variants to accept optinal fd (and try
> > to fallback to sysctl if it's absent/not working)?
>
> both of these ideas were discussed at lsfmm where you were present.
> I'm not sure why you're bring it up again?
Did we actually settle on anything? In that case feel free to ignore me,
maybe I missed that. I remember there were pros/cons for both implementations.
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-25 21:21 UTC (permalink / raw)
To: Andrew Lunn
Cc: Daniel Santos, René van Dorst, sean.wang, f.fainelli, davem,
matthias.bgg, vivien.didelot, frank-w, netdev, linux-mediatek,
linux-mips
In-Reply-To: <20190625204148.GB27733@lunn.ch>
On Tue, Jun 25, 2019 at 10:41:48PM +0200, Andrew Lunn wrote:
> On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
> > On 6/25/19 2:02 PM, Andrew Lunn wrote:
> > >> But will there still be a mechanism to ignore link partner's advertising
> > >> and force these parameters?
> > > >From man 1 ethtool:
> > >
> > > -a --show-pause
> > > Queries the specified Ethernet device for pause parameter information.
> > >
> > > -A --pause
> > > Changes the pause parameters of the specified Ethernet device.
> > >
> > > autoneg on|off
> > > Specifies whether pause autonegotiation should be enabled.
> > >
> > > rx on|off
> > > Specifies whether RX pause should be enabled.
> > >
> > > tx on|off
> > > Specifies whether TX pause should be enabled.
> > >
> > > You need to check the driver to see if it actually implements this
> > > ethtool call, but that is how it should be configured.
> > >
> > > Andrew
> > >
> > Thank you Andrew,
> >
> > So in this context, my question is the difference between "enabling" and
> > "forcing". Here's that register for the mt7620 (which has an mt7530 on
> > its die): https://imgur.com/a/pTk0668 I believe this is also what René
> > is seeking clarity on?
>
> Lets start with normal operation. If the MAC supports pause or asym
> pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
> phylib will then configure the PHY to advertise pause as appropriate.
> Once auto-neg has completed, the results of the negotiation are set in
> phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
> then used to tell the MAC about the autoneg results. The MAC should be
> programmed using the values in phdev->pause and phydev->asym_pause.
>
> For ethtool, the MAC driver needs to implement .get_pauseparam and
> .set_pauseparam. The set_pauseparam needs to validate the settings,
> using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
> tell the PHY about the new configuration. This will trigger a new
> auto-neg if auto-neg is enabled, and the results will be passed back
> in the usual way. If auto-neg is disabled, or pause auto-neg is
> disabled, the MAC should configure pause directly based on the
> settings passed.
>
> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
> want the MAC directly talking to the PHY. Bad things will happen.
> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
> phydev->asym_pause.
>
> The same idea applies when using phylink.
Except when using phylink, use pause & MLO_PAUSE_RX to determine whether
FORCE_RX_FC_Pn should be set, and use pause & MLO_PAUSE_TX to determine
whether FORCE_TX_Pn should be set.
phylink will take care of the results of negotiation with the link
partner and tell you what should be set if pause autoneg is enabled.
If the user has decided to force it via ethtool, and the MAC driver
passes the calls on to phylink, phylink will instead set MLO_PAUSE_RX
and MLO_PAUSE_TX according to the users settings.
So, with phylink, it's quite literally "if MLO_PAUSE_RX is set in
mac_config, enable receiption of pause frames. if MLO_PAUSE_TX is set,
enable transmission of pause frames."
The above applies for phylink's PHY, FIXED, and SGMII in-band modes.
For 802.3z in-band modes, pause is negotiated between the two link
parters (which could be the PCS built into the MACs at either end)
and in some cases its possible to set the MAC to automatically adjust
to the results of the built-in PCS negotiation.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH 1/4] b43legacy: remove b43legacy_dma_set_mask
From: Larry Finger @ 2019-06-25 21:45 UTC (permalink / raw)
To: Christoph Hellwig, Kalle Valo
Cc: b43-dev, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190625102932.32257-2-hch@lst.de>
On 6/25/19 5:29 AM, Christoph Hellwig wrote:
> These days drivers are not required to fallback to smaller DMA masks,
> but can just set the largest mask they support, removing the need for
> this trial and error logic.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/net/wireless/broadcom/b43legacy/dma.c | 39 +------------------
> 1 file changed, 1 insertion(+), 38 deletions(-)
The patches work for PPC32 for both b43legacy and b43.
Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
Thanks,
Larry
>
> diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c
> index 2ce1537d983c..0c2de20622e3 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/dma.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
> @@ -797,43 +797,6 @@ void b43legacy_dma_free(struct b43legacy_wldev *dev)
> dma->tx_ring0 = NULL;
> }
>
> -static int b43legacy_dma_set_mask(struct b43legacy_wldev *dev, u64 mask)
> -{
> - u64 orig_mask = mask;
> - bool fallback = false;
> - int err;
> -
> - /* Try to set the DMA mask. If it fails, try falling back to a
> - * lower mask, as we can always also support a lower one. */
> - while (1) {
> - err = dma_set_mask_and_coherent(dev->dev->dma_dev, mask);
> - if (!err)
> - break;
> - if (mask == DMA_BIT_MASK(64)) {
> - mask = DMA_BIT_MASK(32);
> - fallback = true;
> - continue;
> - }
> - if (mask == DMA_BIT_MASK(32)) {
> - mask = DMA_BIT_MASK(30);
> - fallback = true;
> - continue;
> - }
> - b43legacyerr(dev->wl, "The machine/kernel does not support "
> - "the required %u-bit DMA mask\n",
> - (unsigned int)dma_mask_to_engine_type(orig_mask));
> - return -EOPNOTSUPP;
> - }
> - if (fallback) {
> - b43legacyinfo(dev->wl, "DMA mask fallback from %u-bit to %u-"
> - "bit\n",
> - (unsigned int)dma_mask_to_engine_type(orig_mask),
> - (unsigned int)dma_mask_to_engine_type(mask));
> - }
> -
> - return 0;
> -}
> -
> int b43legacy_dma_init(struct b43legacy_wldev *dev)
> {
> struct b43legacy_dma *dma = &dev->dma;
> @@ -844,7 +807,7 @@ int b43legacy_dma_init(struct b43legacy_wldev *dev)
>
> dmamask = supported_dma_mask(dev);
> type = dma_mask_to_engine_type(dmamask);
> - err = b43legacy_dma_set_mask(dev, dmamask);
> + err = dma_set_mask_and_coherent(dev->dev->dma_dev, dmamask);
> if (err) {
> #ifdef CONFIG_B43LEGACY_PIO
> b43legacywarn(dev->wl, "DMA for this device not supported. "
>
^ permalink raw reply
* BUG: unable to handle kernel NULL pointer dereference in corrupted (4)
From: syzbot @ 2019-06-25 21:47 UTC (permalink / raw)
To: ast, daniel, john.fastabend, linux-kernel, netdev, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 4b972a01 Linux 5.2-rc6
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17852b6ea00000
kernel config: https://syzkaller.appspot.com/x/.config?x=e7c31a94f66cc0aa
dashboard link: https://syzkaller.appspot.com/bug?extid=4b5d77fdf765668f9eba
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15252769a00000
The bug was bisected to:
commit e9db4ef6bf4ca9894bb324c76e01b8f1a16b2650
Author: John Fastabend <john.fastabend@gmail.com>
Date: Sat Jun 30 13:17:47 2018 +0000
bpf: sockhash fix omitted bucket lock in sock_close
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=146ae35ea00000
final crash: https://syzkaller.appspot.com/x/report.txt?x=166ae35ea00000
console output: https://syzkaller.appspot.com/x/log.txt?x=126ae35ea00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4b5d77fdf765668f9eba@syzkaller.appspotmail.com
Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close")
BUG: kernel NULL pointer dereference, address: 00000000000000fc
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [for-next V2 01/10] linux/dim: Move logic to dim.h
From: Sagi Grimberg @ 2019-06-25 21:53 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-2-saeedm@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-25 21:53 UTC (permalink / raw)
To: Vladimir Oltean
Cc: René van Dorst, sean.wang, f.fainelli, davem, matthias.bgg,
andrew, vivien.didelot, frank-w, netdev, linux-mediatek,
linux-mips
In-Reply-To: <6f80325d-4b42-6174-e050-48626f7a3662@gmail.com>
On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> Hi Russell,
>
> On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > This should be removed - state->link is not for use in mac_config.
> > Even in fixed mode, the link can be brought up/down by means of a
> > gpio, and this should be dealt with via the mac_link_* functions.
> >
>
> What do you mean exactly that state->link is not for use, is that true in
> general?
Yes. mac_config() should not touch it; it is not always in a defined
state. For example, if you set modes via ethtool (the
ethtool_ksettings_set API) then state->link will probably contain
zero irrespective of the true link state.
It exists in this structure because it was convenient to just use one
structure to store all the link information in various parts of the
code, and when requesting the negotiated in-band MAC configuration.
I've come to the conclusion that that decision was a mistake, based
on patches such as the above mistakenly thinking that everything in
the state structure is fair game. I've since updated the docs to
explicitly spell it out, but I'm also looking at the feasibility of
changing the mac_config() interface entirely - splitting it into two
(mac_config_fixed() and mac_config_inband()) and passing only the
appropriate parameters to each.
However, having looked at that, I think such a change will make some
MAC drivers quite a bit more complicated - having all the config
steps in one method appears to make the configuration of MAC drivers
easier (eg, mvneta, mvpp2.)
> In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
> (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for
> ports that are BR_STATE_DISABLED. Is that normal?
This looks like another driver which has been converted to phylink
without my review; I certainly wasn't aware of it. It gets a few
things wrong, such as:
1) not checking state->interface in the validate callback - so it
is basically saying that it can support any PHY interface mode
that the kernel happens to support.
2) if phylink is configured to use in-band, then state->speed is
undefined; this driver will fail. (If folk don't want to support
that, we ought to have a way to tell phylink to reject anything
that attempts to set it to in-band mode!)
3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't
support SGMII or 802.3z phy interface modes (see 1).
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [for-next V2 02/10] linux/dim: Remove "net" prefix from internal DIM members
From: Sagi Grimberg @ 2019-06-25 21:53 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-3-saeedm@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [for-next V2 03/10] linux/dim: Rename externally exposed macros
From: Sagi Grimberg @ 2019-06-25 21:54 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-4-saeedm@mellanox.com>
This can be squashed to the prev one, otherwise
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [for-next V2 04/10] linux/dim: Rename net_dim_sample() to net_dim_update_sample()
From: Sagi Grimberg @ 2019-06-25 21:55 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-5-saeedm@mellanox.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* Re: [for-next V2 05/10] linux/dim: Rename externally used net_dim members
From: Sagi Grimberg @ 2019-06-25 21:57 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20190625205701.17849-6-saeedm@mellanox.com>
Question, do any other nics use or plan to use this?
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* [PATCH v4 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-25 21:57 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, Matteo Croce, David S. Miller, Willem de Bruijn
In-Reply-To: <20190619202533.4856-1-nhorman@tuxdriver.com>
When an application is run that:
a) Sets its scheduler to be SCHED_FIFO
and
b) Opens a memory mapped AF_PACKET socket, and sends frames with the
MSG_DONTWAIT flag cleared, its possible for the application to hang
forever in the kernel. This occurs because when waiting, the code in
tpacket_snd calls schedule, which under normal circumstances allows
other tasks to run, including ksoftirqd, which in some cases is
responsible for freeing the transmitted skb (which in AF_PACKET calls a
destructor that flips the status bit of the transmitted frame back to
available, allowing the transmitting task to complete).
However, when the calling application is SCHED_FIFO, its priority is
such that the schedule call immediately places the task back on the cpu,
preventing ksoftirqd from freeing the skb, which in turn prevents the
transmitting task from detecting that the transmission is complete.
We can fix this by converting the schedule call to a completion
mechanism. By using a completion queue, we force the calling task, when
it detects there are no more frames to send, to schedule itself off the
cpu until such time as the last transmitted skb is freed, allowing
forward progress to be made.
Tested by myself and the reporter, with good results
Appies to the net tree
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Matteo Croce <mcroce@redhat.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Change Notes:
V1->V2:
Enhance the sleep logic to support being interruptible and
allowing for honoring to SK_SNDTIMEO (Willem de Bruijn)
V2->V3:
Rearrage the point at which we wait for the completion queue, to
avoid needing to check for ph/skb being null at the end of the loop.
Also move the complete call to the skb destructor to avoid needing to
modify __packet_set_status. Also gate calling complete on
packet_read_pending returning zero to avoid multiple calls to complete.
(Willem de Bruijn)
Move timeo computation within loop, to re-fetch the socket
timeout since we also use the timeo variable to record the return code
from the wait_for_complete call (Neil Horman)
V3->V4:
Willem has requested that the control flow be restored to the
previous state. Doing so lets us eliminate the need for the
po->wait_on_complete flag variable, and lets us get rid of the
packet_next_frame function, but introduces another complexity.
Specifically, but using the packet pending count, we can, if an
applications calls sendmsg multiple times with MSG_DONTWAIT set, each
set of transmitted frames, when complete, will cause
tpacket_destruct_skb to issue a complete call, for which there will
never be a wait_on_completion call. This imbalance will lead to any
future call to wait_for_completion here to return early, when the frames
they sent may not have completed. To correct this, we need to re-init
the completion queue on every call to tpacket_snd before we enter the
loop so as to ensure we wait properly for the frames we send in this
iteration.
Change the timeout and interrupted gotos to out_put rather than
out_status so that we don't try to free a non-existant skb
Clean up some extra newlines (Willem de Bruijn)
---
net/packet/af_packet.c | 20 +++++++++++++++++---
net/packet/internal.h | 1 +
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a29d66da7394..a7ca6a003ebe 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2401,6 +2401,9 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
ts = __packet_set_timestamp(po, ph, skb);
__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
+
+ if (!packet_read_pending(&po->tx_ring))
+ complete(&po->skb_completion);
}
sock_wfree(skb);
@@ -2585,7 +2588,7 @@ static int tpacket_parse_header(struct packet_sock *po, void *frame,
static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
{
- struct sk_buff *skb;
+ struct sk_buff *skb = NULL;
struct net_device *dev;
struct virtio_net_hdr *vnet_hdr = NULL;
struct sockcm_cookie sockc;
@@ -2600,6 +2603,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
int len_sum = 0;
int status = TP_STATUS_AVAILABLE;
int hlen, tlen, copylen = 0;
+ long timeo = 0;
mutex_lock(&po->pg_vec_lock);
@@ -2646,12 +2650,21 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !po->has_vnet_hdr)
size_max = dev->mtu + reserve + VLAN_HLEN;
+ reinit_completion(&po->skb_completion);
+
do {
ph = packet_current_frame(po, &po->tx_ring,
TP_STATUS_SEND_REQUEST);
if (unlikely(ph == NULL)) {
- if (need_wait && need_resched())
- schedule();
+ if (need_wait && skb) {
+ timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
+ timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
+ if (timeo <= 0) {
+ err = !timeo ? -ETIMEDOUT : -ERESTARTSYS;
+ goto out_put;
+ }
+ }
+ /* check for additional frames */
continue;
}
@@ -3207,6 +3220,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol,
sock_init_data(sock, sk);
po = pkt_sk(sk);
+ init_completion(&po->skb_completion);
sk->sk_family = PF_PACKET;
po->num = proto;
po->xmit = dev_queue_xmit;
diff --git a/net/packet/internal.h b/net/packet/internal.h
index 3bb7c5fb3bff..c70a2794456f 100644
--- a/net/packet/internal.h
+++ b/net/packet/internal.h
@@ -128,6 +128,7 @@ struct packet_sock {
unsigned int tp_hdrlen;
unsigned int tp_reserve;
unsigned int tp_tstamp;
+ struct completion skb_completion;
struct net_device __rcu *cached_dev;
int (*xmit)(struct sk_buff *skb);
struct packet_type prot_hook ____cacheline_aligned_in_smp;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-25 21:59 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <20190625162028.GC29902@hmswarspite.think-freely.org>
On Tue, Jun 25, 2019 at 12:20 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Tue, Jun 25, 2019 at 09:37:17AM -0400, Willem de Bruijn wrote:
> > On Tue, Jun 25, 2019 at 7:03 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Mon, Jun 24, 2019 at 06:15:29PM -0400, Willem de Bruijn wrote:
> > > > > > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > > > > > + po->wait_on_complete = 1;
> > > > > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > > > > >
> > > > > > This resets timeout on every loop. should only set above the loop once.
> > > > > >
> > > > > I explained exactly why I did that in the change log. Its because I reuse the
> > > > > timeout variable to get the return value of the wait_for_complete call.
> > > > > Otherwise I need to add additional data to the stack, which I don't want to do.
> > > > > Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> > > > > path, so I see no reason not to reuse the variable.
> > > >
> > > > The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
> > > > each time. Whereas wait_for_common and its variants return the
> > > > number of jiffies left in case the loop needs to sleep again later.
> > > >
> > > > Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
> > > > common pattern across the stack.
> > > >
> > > But those patterns are unique to those situations. For instance, in
> > > tcp_sendmsg_locked, we aquire the value of the socket timeout, and use that to
> > > wait for the entire message send operation to complete, which consists of
> > > potentially several blocking operations (waiting for the tcp connection to be
> > > established, waiting for socket memory, etc). In that situation we want to wait
> > > for all of those operations to complete to send a single message, and fail if
> > > they exceed the timeout in aggregate. The semantics are different with
> > > AF_PACKET. In this use case, the message is in effect empty, and just used to
> > > pass some control information. tpacket_snd, sends as many frames from the
> > > memory mapped buffer as possible, and on each iteration we want to wait for the
> > > specified timeout for those frames to complete sending. I think resetting the
> > > timeout on each wait instance is the right way to go here.
> >
> > I disagree. If a SO_SNDTIMEO of a given time is set, the thread should
> > not wait beyond that. Else what is the point of passing a specific
> > duration in the syscall?
> >
> I can appreciate that, but you said yourself that you wanted to minimize this
> change. The current behavior of AF_PACKET is to:
> a) check for their being no more packets ready to send
> b) if (a) is false we schedule() (nominally allowing other tasks to run)
> c) when (b) is complete, check for additional frames to send, and exit if there
> are not, otherwise, continue sending and waiting
>
> In that model, a single task calling sendmsg can run in the kernel indefinately,
> if userspace continues to fill the memory mapped buffer
>
> Given that model, resetting the timeout on each call to wait_for_completion
> keeps that behavior. In fact, if we allow the timeout value to get continuously
> decremented, it will be possible for a call to sendmsg in this protocol to
> _always_ return -ETIMEDOUT (if we keep the buffer full in userspace long enough,
> then the sending task will eventually decrement timeo to zero, and force an
> -ETIMEDOUT call).
>
> I'm convinced that resetting the timeout here is the right way to go.
It upholds the contract, but extends when userspace appends to the
ring. Okay, yes, that makes sense.
> > Btw, we can always drop the timeo and go back to unconditional (bar
> > signals) waiting.
> >
> We could, but it would be nice to implement the timeout feature here if
> possible.
>
> > >
> > > > > > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > > > > err = net_xmit_errno(err);
> > > > > > > if (err && __packet_get_status(po, ph) ==
> > > > > > > TP_STATUS_AVAILABLE) {
> > > > > > > + /* re-init completion queue to avoid subsequent fallthrough
> > > > > > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > > > > > + */
> > > > > > > + po->wait_on_complete = 0;
> > > > > >
> > > > > > If setting where sleeping, no need for resetting if a failure happens
> > > > > > between those blocks.
> > > > > >
> > > > > > > + init_completion(&po->skb_completion);
> > > > > >
> > > > > > no need to reinit between each use?
> > > > > >
> > > > > I explained exactly why I did this in the comment above. We have to set
> > > > > wait_for_complete prior to calling transmit, so as to ensure that we call
> > > > > wait_for_completion before we exit the loop. However, in this error case, we
> > > > > exit the loop prior to calling wait_for_complete, so we need to reset the
> > > > > completion variable and the wait_for_complete flag. Otherwise we will be in a
> > > > > case where, on the next entrace to this loop we will have a completion variable
> > > > > with completion->done > 0, meaning the next wait will be a fall through case,
> > > > > which we don't want.
> > > >
> > > > By moving back to the point where schedule() is called, hopefully this
> > > > complexity automatically goes away. Same as my comment to the line
> > > > immediately above.
> > > >
> > > Its going to change what the complexity is, actually. I was looking at this
> > > last night, and I realized that your assertion that we could remove
> > > packet_next_frame came at a cost. This is because we need to determine if we
> > > have to wait before we call po->xmit, but we need to actually do the wait after
> > > po->xmit
> >
> > Why? The existing method using schedule() doesn't.
> >
> Because the existing method using schedule doesn't have to rely on an
> asynchronous event to wake the sending task back up. Specifically, we need to
> set some state that indicates tpacket_destruct_skb should call complete, and
> since tpacket_destruct_skb is called asynchronously in a separate execution
> context, we need to ensure there is no race condition whereby we execute
> tpacket_destruct_skb before we set the state that we also use to determine if we
> should call wait_for_complete. ie:
> 1) tpacket_send_skb calls po->xmit
> 2) tpacket_send_skb loops around and checks to see if there are more packets to
> send. If not and if need_wait is set we call wait_for_complete
>
> If tpacket_destruct_skb is called after (2), we're fine. But if its called
> between (1) and (2), then tpacket_destruct_skb won't call complete (because
> wait_on_complete isn't set yet), causing a hang.
>
> Because you wanted to remove packet_next_frame, we have no way to determine if
> we're done sending frames prior to calling po->xmit, which is the point past
> which tpacket_destruct_skb might be called, so now I have to find an alternate
> state condition to key off of.
>
>
> > Can we not just loop while sending and sleep immediately when
> > __packet_get_status returns !TP_STATUS_AVAILABLE?
> >
> > I don't understand the need to probe the next packet to send instead
> > of the current.
> >
> See above, we can definately check the return of ph at the top of the loop, and
> sleep if its NULL, but in so doing we cant use po->wait_on_complete as a gating
> variable because we've already called po->xmit. Once we call that function, if
> we haven't already set po->wait_on_complete to true, its possible
> tpacket_destruct_skb will already have been called before we get to the point
> where we check wait_for_completion. That means we will wait on a completion
> variable that never gets completed, so I need to find a new way to track weather
> or not we are waiting. Its entirely doable, I'm sure, its just not as straight
> forward as your making it out to be.
>
> > This seems to be the crux of the disagreement. My guess is that it has
> > to do with setting wait_on_complete, but I don't see the problem. It
> > can be set immediately before going to sleep.
> >
> The reason has to do with the fact that
> tpacket_destruct_skb can be called from ksoftirq context (i.e. it will be called
> asynchrnously, not from the thread running in tpacket_snd). Condsider this
> flow, assuming we do as you suggest, and just set po->wait_on_complete=1
> immediately prior to calling wait_for_complete_interruptible_timeout:
>
> 1) tpacket_snd gets a frame, builds the skb for transmission
> 2) tpakcket_snd calls po->xmit(skb), sending the frame to the driver
> 3) tpacket_snd, iterates through back to the top of the loop, and determines
> that we need to wait for the previous packet to complete
> 4) The driver gets a tx completion interrupt, interrupting the cpu on which we
> are executing, and calls kfree_skb_irq on the skb we just transmitted
> 5) the ksoftirq runs on the cpu, calling kfree_skb on our skb
> 6) (5) calls skb->destruct (which is tpakcet_skb_destruct)
> 7) tpacket_skb_destruct executes, sees that po->wait_on_completion is zero, and
> skips calling complete()
> 8) the thread running tpacket_snd gets scheduled back on the cpu and now sets
> po->wait_on_complete, then calls wait_for_completion_interruptible_timeout, but
> since the skb we are waiting to complete has already been freed, we will never
> get the completion notification, and so we will wait for the maximal timeout,
> which is absolutely not what we want.
>
> Interestingly (Ironically), that control flow will never happen in the use case
> I'm trying to fix here, because its SCHED_FIFO, and will run until such time as
> we call wait_for_completion_interuptible_timeout, so in this case we're safe.
> But in the nominal case, where the sending task is acturally SCHED_OTHER, the
> above can aboslutely happen, and thats very broken.
>
> > I don't meant to draw this out, btw, or add to your workload. If you
> > prefer, I can take a stab at my (admittedly hand-wavy) suggestion.
> >
> No, I have another method in flight that I'm testing now, it removes the
> po->wait_on_complete variable entirely, checking instead to make sure that
> we've:
> a) sent at least one frame
> and
> b) that we have a positive pending count
> and
> c) that we need to wait
> and
> d) that we have no more frames to send
>
> This is why I was saying that your idea can be done, but it trades one form of
> complexity for another.
Thanks for the detailed explanation of the races.
I was thinking this through, but just see V4. Will take a look at that.
^ permalink raw reply
* Re: [for-next V2 08/10] linux/dim: Implement rdma_dim
From: Sagi Grimberg @ 2019-06-25 22:02 UTC (permalink / raw)
To: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman, Max Gurtovoy
In-Reply-To: <20190625205701.17849-9-saeedm@mellanox.com>
> +void rdma_dim(struct dim *dim, u64 completions)
> +{
> + struct dim_sample *curr_sample = &dim->measuring_sample;
> + struct dim_stats curr_stats;
> + u32 nevents;
> +
> + dim_update_sample_with_comps(curr_sample->event_ctr + 1,
> + curr_sample->pkt_ctr,
> + curr_sample->byte_ctr,
> + curr_sample->comp_ctr + completions,
> + &dim->measuring_sample);
If this is the only caller, why add pkt_ctr and byte_ctr at all?
^ permalink raw reply
* [PATCH bpf-next v9 01/10] fs,security: Add a new file access type: MAY_CHROOT
From: Mickaël Salaün @ 2019-06-25 21:52 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Aleksa Sarai, Alexander Viro,
Alexei Starovoitov, Andrew Morton, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <20190625215239.11136-1-mic@digikod.net>
For compatibility reason, MAY_CHROOT is always set with MAY_CHDIR.
However, this new flag enable to differentiate a chdir form a chroot.
This is needed for the Landlock LSM to be able to evaluate a new root
directory.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: James Morris <jmorris@namei.org>
Cc: John Johansen <john.johansen@canonical.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: linux-fsdevel@vger.kernel.org
---
fs/open.c | 3 ++-
include/linux/fs.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/open.c b/fs/open.c
index b5b80469b93d..e8767318fd03 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -494,7 +494,8 @@ int ksys_chroot(const char __user *filename)
if (error)
goto out;
- error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR);
+ error = inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR |
+ MAY_CHROOT);
if (error)
goto dput_and_out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f7fdfe93e25d..fa6c9d0c152b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -99,6 +99,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define MAY_CHDIR 0x00000040
/* called from RCU mode, don't block */
#define MAY_NOT_BLOCK 0x00000080
+#define MAY_CHROOT 0x00000100
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v9 07/10] landlock: Add ptrace restrictions
From: Mickaël Salaün @ 2019-06-25 21:52 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Aleksa Sarai, Alexander Viro,
Alexei Starovoitov, Andrew Morton, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <20190625215239.11136-1-mic@digikod.net>
A landlocked process has less privileges than a non-landlocked process
and must then be subject to additional restrictions when manipulating
processes. To be allowed to use ptrace(2) and related syscalls on a
target process, a landlocked process must have a subset of the target
process' rules.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: James Morris <jmorris@namei.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
---
Changes since v6:
* factor out ptrace check
* constify pointers
* cleanup headers
* use the new security_add_hooks()
---
security/landlock/Makefile | 2 +-
security/landlock/hooks_ptrace.c | 121 +++++++++++++++++++++++++++++++
security/landlock/hooks_ptrace.h | 8 ++
security/landlock/init.c | 2 +
4 files changed, 132 insertions(+), 1 deletion(-)
create mode 100644 security/landlock/hooks_ptrace.c
create mode 100644 security/landlock/hooks_ptrace.h
diff --git a/security/landlock/Makefile b/security/landlock/Makefile
index 270ece5d93de..4500ddb0767e 100644
--- a/security/landlock/Makefile
+++ b/security/landlock/Makefile
@@ -2,4 +2,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
landlock-y := init.o \
enforce.o enforce_seccomp.o \
- hooks.o hooks_fs.o
+ hooks.o hooks_fs.o hooks_ptrace.o
diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
new file mode 100644
index 000000000000..7f5e8b994e93
--- /dev/null
+++ b/security/landlock/hooks_ptrace.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Landlock LSM - ptrace hooks
+ *
+ * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
+ */
+
+#include <asm/current.h>
+#include <linux/errno.h>
+#include <linux/kernel.h> /* ARRAY_SIZE */
+#include <linux/lsm_hooks.h>
+#include <linux/sched.h> /* struct task_struct */
+#include <linux/seccomp.h>
+
+#include "common.h" /* struct landlock_prog_set */
+#include "hooks.h" /* landlocked() */
+#include "hooks_ptrace.h"
+
+static bool progs_are_subset(const struct landlock_prog_set *parent,
+ const struct landlock_prog_set *child)
+{
+ size_t i;
+
+ if (!parent || !child)
+ return false;
+ if (parent == child)
+ return true;
+
+ for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
+ struct landlock_prog_list *walker;
+ bool found_parent = false;
+
+ if (!parent->programs[i])
+ continue;
+ for (walker = child->programs[i]; walker;
+ walker = walker->prev) {
+ if (walker == parent->programs[i]) {
+ found_parent = true;
+ break;
+ }
+ }
+ if (!found_parent)
+ return false;
+ }
+ return true;
+}
+
+static bool task_has_subset_progs(const struct task_struct *parent,
+ const struct task_struct *child)
+{
+#ifdef CONFIG_SECCOMP_FILTER
+ if (progs_are_subset(parent->seccomp.landlock_prog_set,
+ child->seccomp.landlock_prog_set))
+ /* must be ANDed with other providers (i.e. cgroup) */
+ return true;
+#endif /* CONFIG_SECCOMP_FILTER */
+ return false;
+}
+
+static int task_ptrace(const struct task_struct *parent,
+ const struct task_struct *child)
+{
+ if (!landlocked(parent))
+ return 0;
+
+ if (!landlocked(child))
+ return -EPERM;
+
+ if (task_has_subset_progs(parent, child))
+ return 0;
+
+ return -EPERM;
+}
+
+/**
+ * hook_ptrace_access_check - determine whether the current process may access
+ * another
+ *
+ * @child: the process to be accessed
+ * @mode: the mode of attachment
+ *
+ * If the current task has Landlock programs, then the child must have at least
+ * the same programs. Else denied.
+ *
+ * Determine whether a process may access another, returning 0 if permission
+ * granted, -errno if denied.
+ */
+static int hook_ptrace_access_check(struct task_struct *child,
+ unsigned int mode)
+{
+ return task_ptrace(current, child);
+}
+
+/**
+ * hook_ptrace_traceme - determine whether another process may trace the
+ * current one
+ *
+ * @parent: the task proposed to be the tracer
+ *
+ * If the parent has Landlock programs, then the current task must have the
+ * same or more programs.
+ * Else denied.
+ *
+ * Determine whether the nominated task is permitted to trace the current
+ * process, returning 0 if permission is granted, -errno if denied.
+ */
+static int hook_ptrace_traceme(struct task_struct *parent)
+{
+ return task_ptrace(parent, current);
+}
+
+static struct security_hook_list landlock_hooks[] = {
+ LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
+ LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
+};
+
+__init void landlock_add_hooks_ptrace(void)
+{
+ security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
+ LANDLOCK_NAME);
+}
diff --git a/security/landlock/hooks_ptrace.h b/security/landlock/hooks_ptrace.h
new file mode 100644
index 000000000000..2c2b8a13037f
--- /dev/null
+++ b/security/landlock/hooks_ptrace.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Landlock LSM - ptrace hooks
+ *
+ * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
+ */
+
+__init void landlock_add_hooks_ptrace(void);
diff --git a/security/landlock/init.c b/security/landlock/init.c
index 68def2a7af71..d58aa94124b0 100644
--- a/security/landlock/init.c
+++ b/security/landlock/init.c
@@ -13,6 +13,7 @@
#include "common.h" /* LANDLOCK_* */
#include "hooks_fs.h"
+#include "hooks_ptrace.h"
static bool bpf_landlock_is_valid_access(int off, int size,
enum bpf_access_type type, const struct bpf_prog *prog,
@@ -143,6 +144,7 @@ const struct bpf_prog_ops landlock_prog_ops = {};
static int __init landlock_init(void)
{
pr_info(LANDLOCK_NAME ": Initializing (sandbox with seccomp)\n");
+ landlock_add_hooks_ptrace();
landlock_add_hooks_fs();
return 0;
}
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v9 10/10] landlock: Add user and kernel documentation for Landlock
From: Mickaël Salaün @ 2019-06-25 21:52 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Aleksa Sarai, Alexander Viro,
Alexei Starovoitov, Andrew Morton, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <20190625215239.11136-1-mic@digikod.net>
This documentation can be built with the Sphinx framework.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: James Morris <jmorris@namei.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
---
Changes since v8:
* remove documentation related to chaining and tagging according to this
patch series
Changes since v7:
* update documentation according to the Landlock revamp
Changes since v6:
* add a check for ctx->event
* rename BPF_PROG_TYPE_LANDLOCK to BPF_PROG_TYPE_LANDLOCK_RULE
* rename Landlock version to ABI to better reflect its purpose and add a
dedicated changelog section
* update tables
* relax no_new_privs recommendations
* remove ABILITY_WRITE related functions
* reword rule "appending" to "prepending" and explain it
* cosmetic fixes
Changes since v5:
* update the rule hierarchy inheritance explanation
* briefly explain ctx->arg2
* add ptrace restrictions
* explain EPERM
* update example (subtype)
* use ":manpage:"
---
Documentation/security/index.rst | 1 +
Documentation/security/landlock/index.rst | 20 +++
Documentation/security/landlock/kernel.rst | 99 ++++++++++++++
Documentation/security/landlock/user.rst | 148 +++++++++++++++++++++
4 files changed, 268 insertions(+)
create mode 100644 Documentation/security/landlock/index.rst
create mode 100644 Documentation/security/landlock/kernel.rst
create mode 100644 Documentation/security/landlock/user.rst
diff --git a/Documentation/security/index.rst b/Documentation/security/index.rst
index aad6d92ffe31..32b4c1db2325 100644
--- a/Documentation/security/index.rst
+++ b/Documentation/security/index.rst
@@ -12,3 +12,4 @@ Security Documentation
SCTP
self-protection
tpm/index
+ landlock/index
diff --git a/Documentation/security/landlock/index.rst b/Documentation/security/landlock/index.rst
new file mode 100644
index 000000000000..d0af868d1582
--- /dev/null
+++ b/Documentation/security/landlock/index.rst
@@ -0,0 +1,20 @@
+=========================================
+Landlock LSM: programmatic access control
+=========================================
+
+Landlock is a stackable Linux Security Module (LSM) that makes it possible to
+create security sandboxes, programmable access-controls or safe endpoint
+security agents. This kind of sandbox is expected to help mitigate the
+security impact of bugs or unexpected/malicious behaviors in user-space
+applications. The current version allows only a process with the global
+CAP_SYS_ADMIN capability to create such sandboxes but the ultimate goal of
+Landlock is to empower any process, including unprivileged ones, to securely
+restrict themselves. Landlock is inspired by seccomp-bpf but instead of
+filtering syscalls and their raw arguments, a Landlock rule can inspect the use
+of kernel objects like files and hence make a decision according to the kernel
+semantic.
+
+.. toctree::
+
+ user
+ kernel
diff --git a/Documentation/security/landlock/kernel.rst b/Documentation/security/landlock/kernel.rst
new file mode 100644
index 000000000000..7d1e06d544bf
--- /dev/null
+++ b/Documentation/security/landlock/kernel.rst
@@ -0,0 +1,99 @@
+==============================
+Landlock: kernel documentation
+==============================
+
+eBPF properties
+===============
+
+To get an expressive language while still being safe and small, Landlock is
+based on eBPF. Landlock should be usable by untrusted processes and must
+therefore expose a minimal attack surface. The eBPF bytecode is minimal,
+powerful, widely used and designed to be used by untrusted applications. Thus,
+reusing the eBPF support in the kernel enables a generic approach while
+minimizing new code.
+
+An eBPF program has access to an eBPF context containing some fields used to
+inspect the current object. These arguments can be used directly (e.g. cookie)
+or passed to helper functions according to their types (e.g. inode pointer). It
+is then possible to do complex access checks without race conditions or
+inconsistent evaluation (i.e. `incorrect mirroring of the OS code and state
+<https://www.ndss-symposium.org/ndss2003/traps-and-pitfalls-practical-problems-system-call-interposition-based-security-tools/>`_).
+
+A Landlock hook describes a particular access type. For now, there is two
+hooks dedicated to filesystem related operations: LANDLOCK_HOOK_FS_PICK and
+LANDLOCK_HOOK_FS_WALK. A Landlock program is tied to one hook. This makes it
+possible to statically check context accesses, potentially performed by such
+program, and hence prevents kernel address leaks and ensure the right use of
+hook arguments with eBPF functions. Any user can add multiple Landlock
+programs per Landlock hook. They are stacked and evaluated one after the
+other, starting from the most recent program, as seccomp-bpf does with its
+filters. Underneath, a hook is an abstraction over a set of LSM hooks.
+
+
+Guiding principles
+==================
+
+Unprivileged use
+----------------
+
+* Landlock helpers and context should be usable by any unprivileged and
+ untrusted program while following the system security policy enforced by
+ other access control mechanisms (e.g. DAC, LSM).
+
+
+Landlock hook and context
+-------------------------
+
+* A Landlock hook shall be focused on access control on kernel objects instead
+ of syscall filtering (i.e. syscall arguments), which is the purpose of
+ seccomp-bpf.
+* A Landlock context provided by a hook shall express the minimal and more
+ generic interface to control an access for a kernel object.
+* A hook shall guaranty that all the BPF function calls from a program are
+ safe. Thus, the related Landlock context arguments shall always be of the
+ same type for a particular hook. For example, a network hook could share
+ helpers with a file hook because of UNIX socket. However, the same helpers
+ may not be compatible for a file system handle and a net handle.
+* Multiple hooks may use the same context interface.
+
+
+Landlock helpers
+----------------
+
+* Landlock helpers shall be as generic as possible while at the same time being
+ as simple as possible and following the syscall creation principles (cf.
+ *Documentation/adding-syscalls.txt*).
+* The only behavior change allowed on a helper is to fix a (logical) bug to
+ match the initial semantic.
+* Helpers shall be reentrant, i.e. only take inputs from arguments (e.g. from
+ the BPF context), to enable a hook to use a cache. Future program options
+ might change this cache behavior.
+* It is quite easy to add new helpers to extend Landlock. The main concern
+ should be about the possibility to leak information from the kernel that may
+ not be accessible otherwise (i.e. side-channel attack).
+
+
+Questions and answers
+=====================
+
+Why not create a custom hook for each kind of action?
+-----------------------------------------------------
+
+Landlock programs can handle these checks. Adding more exceptions to the
+kernel code would lead to more code complexity. A decision to ignore a kind of
+action can and should be done at the beginning of a Landlock program.
+
+
+Why a program does not return an errno or a kill code?
+------------------------------------------------------
+
+seccomp filters can return multiple kind of code, including an errno value or a
+kill signal, which may be convenient for access control. Those return codes
+are hardwired in the userland ABI. Instead, Landlock's approach is to return a
+boolean to allow or deny an action, which is much simpler and more generic.
+Moreover, we do not really have a choice because, unlike to seccomp, Landlock
+programs are not enforced at the syscall entry point but may be executed at any
+point in the kernel (through LSM hooks) where an errno return code may not make
+sense. However, with this simple ABI and with the ability to call helpers,
+Landlock may gain features similar to seccomp-bpf in the future while being
+compatible with previous programs.
diff --git a/Documentation/security/landlock/user.rst b/Documentation/security/landlock/user.rst
new file mode 100644
index 000000000000..e4bb7dba4aa7
--- /dev/null
+++ b/Documentation/security/landlock/user.rst
@@ -0,0 +1,148 @@
+================================
+Landlock: userland documentation
+================================
+
+Landlock programs
+=================
+
+eBPF programs are used to create security programs. They are contained and can
+call only a whitelist of dedicated functions. Moreover, they can only loop
+under strict conditions, which protects from denial of service. More
+information on BPF can be found in *Documentation/networking/filter.txt*.
+
+
+Writing a program
+-----------------
+
+To enforce a security policy, a thread first needs to create a Landlock program.
+The easiest way to write an eBPF program depicting a security program is to write
+it in the C language. As described in *samples/bpf/README.rst*, LLVM can
+compile such programs. Files *samples/bpf/landlock1_kern.c* and those in
+*tools/testing/selftests/landlock/* can be used as examples.
+
+Once the eBPF program is created, the next step is to create the metadata
+describing the Landlock program. This metadata includes a subtype which
+contains the hook type to which the program is tied and some options.
+
+.. code-block:: c
+
+ union bpf_prog_subtype subtype = {
+ .landlock_hook = {
+ .type = LANDLOCK_HOOK_FS_PICK,
+ .triggers = LANDLOCK_TRIGGER_FS_PICK_OPEN,
+ }
+ };
+
+A Landlock hook describes the kind of kernel object for which a program will be
+triggered to allow or deny an action. For example, the hook
+LANDLOCK_HOOK_FS_PICK can be triggered every time a landlocked thread performs
+a set of action related to the filesystem (e.g. open, read, write, mount...).
+This actions are identified by the `triggers` bitfield.
+
+The next step is to fill a :c:type:`union bpf_attr <bpf_attr>` with
+BPF_PROG_TYPE_LANDLOCK_HOOK, the previously created subtype and other BPF
+program metadata. This bpf_attr must then be passed to the :manpage:`bpf(2)`
+syscall alongside the BPF_PROG_LOAD command. If everything is deemed correct
+by the kernel, the thread gets a file descriptor referring to this program.
+
+In the following code, the *insn* variable is an array of BPF instructions
+which can be extracted from an ELF file as is done in bpf_load_file() from
+*samples/bpf/bpf_load.c*.
+
+.. code-block:: c
+
+ union bpf_attr attr = {
+ .prog_type = BPF_PROG_TYPE_LANDLOCK_HOOK,
+ .insn_cnt = sizeof(insn) / sizeof(struct bpf_insn),
+ .insns = (__u64) (unsigned long) insn,
+ .license = (__u64) (unsigned long) "GPL",
+ .prog_subtype = &subtype,
+ .prog_subtype_size = sizeof(subtype),
+ };
+ int fd = bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
+ if (fd == -1)
+ exit(1);
+
+
+Enforcing a program
+-------------------
+
+Once the Landlock program has been created or received (e.g. through a UNIX
+socket), the thread willing to sandbox itself (and its future children) should
+perform the following two steps.
+
+The thread should first request to never be allowed to get new privileges with a
+call to :manpage:`prctl(2)` and the PR_SET_NO_NEW_PRIVS option. More
+information can be found in *Documentation/prctl/no_new_privs.txt*.
+
+.. code-block:: c
+
+ if (prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0))
+ exit(1);
+
+A thread can apply a program to itself by using the :manpage:`seccomp(2)` syscall.
+The operation is SECCOMP_PREPEND_LANDLOCK_PROG, the flags must be empty and the
+*args* argument must point to a valid Landlock program file descriptor.
+
+.. code-block:: c
+
+ if (seccomp(SECCOMP_PREPEND_LANDLOCK_PROG, 0, &fd))
+ exit(1);
+
+If the syscall succeeds, the program is now enforced on the calling thread and
+will be enforced on all its subsequently created children of the thread as
+well. Once a thread is landlocked, there is no way to remove this security
+policy, only stacking more restrictions is allowed. The program evaluation is
+performed from the newest to the oldest.
+
+When a syscall ask for an action on a kernel object, if this action is denied,
+then an EACCES errno code is returned through the syscall.
+
+
+.. _inherited_programs:
+
+Inherited programs
+------------------
+
+Every new thread resulting from a :manpage:`clone(2)` inherits Landlock program
+restrictions from its parent. This is similar to the seccomp inheritance as
+described in *Documentation/prctl/seccomp_filter.txt*.
+
+
+Ptrace restrictions
+-------------------
+
+A landlocked process has less privileges than a non-landlocked process and must
+then be subject to additional restrictions when manipulating another process.
+To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target
+process, a landlocked process must have a subset of the target process programs.
+
+
+Landlock structures and constants
+=================================
+
+Hook types
+----------
+
+.. kernel-doc:: include/uapi/linux/landlock.h
+ :functions: landlock_hook_type
+
+
+Contexts
+--------
+
+.. kernel-doc:: include/uapi/linux/landlock.h
+ :functions: landlock_ctx_fs_pick landlock_ctx_fs_walk landlock_ctx_fs_get
+
+
+Triggers for fs_pick
+--------------------
+
+.. kernel-doc:: include/uapi/linux/landlock.h
+ :functions: landlock_triggers
+
+
+Additional documentation
+========================
+
+See https://landlock.io
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v9 08/10] bpf: Add a Landlock sandbox example
From: Mickaël Salaün @ 2019-06-25 21:52 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Aleksa Sarai, Alexander Viro,
Alexei Starovoitov, Andrew Morton, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <20190625215239.11136-1-mic@digikod.net>
Add a basic sandbox tool to launch a command which is denied access to a
list of files and directories.
Add to the bpf_load library the ability to handle a BPF program subtype.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: James Morris <jmorris@namei.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
---
Changes since v8:
* rewrite the landlock1 sample which deny access to a set of files or
directories (i.e. simple blacklist) to fit with the previous patches
* add "landlock1" to .gitignore
* in bpf_load.c, pass the subtype with a call to
bpf_load_program_xattr()
Changes since v7:
* rewrite the example using an inode map
* add to bpf_load the ability to handle subtypes per program type
Changes since v6:
* check return value of load_and_attach()
* allow to write on pipes
* rename BPF_PROG_TYPE_LANDLOCK to BPF_PROG_TYPE_LANDLOCK_RULE
* rename Landlock version to ABI to better reflect its purpose
* use const variable (suggested by Kees Cook)
* remove useless definitions (suggested by Kees Cook)
* add detailed explanations (suggested by Kees Cook)
Changes since v5:
* cosmetic fixes
* rebase
Changes since v4:
* write Landlock rule in C and compiled it with LLVM
* remove cgroup handling
* remove path handling: only handle a read-only environment
* remove errno return codes
Changes since v3:
* remove seccomp and origin field: completely free from seccomp programs
* handle more FS-related hooks
* handle inode hooks and directory traversal
* add faked but consistent view thanks to ENOENT
* add /lib64 in the example
* fix spelling
* rename some types and definitions (e.g. SECCOMP_ADD_LANDLOCK_RULE)
Changes since v2:
* use BPF_PROG_ATTACH for cgroup handling
---
samples/bpf/.gitignore | 1 +
samples/bpf/Makefile | 3 +
samples/bpf/bpf_load.c | 76 ++++++++++++++++-
samples/bpf/bpf_load.h | 7 ++
samples/bpf/landlock1.h | 8 ++
samples/bpf/landlock1_kern.c | 104 +++++++++++++++++++++++
samples/bpf/landlock1_user.c | 157 +++++++++++++++++++++++++++++++++++
7 files changed, 352 insertions(+), 4 deletions(-)
create mode 100644 samples/bpf/landlock1.h
create mode 100644 samples/bpf/landlock1_kern.c
create mode 100644 samples/bpf/landlock1_user.c
diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
index 74d31fd3c99c..a4c9c806f739 100644
--- a/samples/bpf/.gitignore
+++ b/samples/bpf/.gitignore
@@ -2,6 +2,7 @@ cpustat
fds_example
hbm
ibumad
+landlock1
lathist
lwt_len_hist
map_perf_test
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 0917f8cf4fab..da246eaa8bf8 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -53,6 +53,7 @@ hostprogs-y += task_fd_query
hostprogs-y += xdp_sample_pkts
hostprogs-y += ibumad
hostprogs-y += hbm
+hostprogs-y += landlock1
# Libbpf dependencies
LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
@@ -109,6 +110,7 @@ task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS)
xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS)
ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS)
hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS)
+landlock1-objs := bpf_load.o landlock1_user.o
# Tell kbuild to always build the programs
always := $(hostprogs-y)
@@ -168,6 +170,7 @@ always += task_fd_query_kern.o
always += xdp_sample_pkts_kern.o
always += ibumad_kern.o
always += hbm_out_kern.o
+always += landlock1_kern.o
KBUILD_HOSTCFLAGS += -I$(objtree)/usr/include
KBUILD_HOSTCFLAGS += -I$(srctree)/tools/lib/bpf/
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 4574b1939e49..bf62d965f606 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -12,6 +12,7 @@
#include <stdlib.h>
#include <linux/bpf.h>
#include <linux/filter.h>
+#include <linux/landlock.h>
#include <linux/perf_event.h>
#include <linux/netlink.h>
#include <linux/rtnetlink.h>
@@ -42,6 +43,9 @@ int prog_array_fd = -1;
struct bpf_map_data map_data[MAX_MAPS];
int map_data_count;
+struct bpf_subtype_data subtype_data[MAX_PROGS];
+int subtype_data_count;
+
static int populate_prog_array(const char *event, int prog_fd)
{
int ind = atoi(event), err;
@@ -87,11 +91,15 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
bool is_sockops = strncmp(event, "sockops", 7) == 0;
bool is_sk_skb = strncmp(event, "sk_skb", 6) == 0;
bool is_sk_msg = strncmp(event, "sk_msg", 6) == 0;
+ bool is_landlock = strncmp(event, "landlock", 8) == 0;
size_t insns_cnt = size / sizeof(struct bpf_insn);
enum bpf_prog_type prog_type;
char buf[256];
int fd, efd, err, id;
struct perf_event_attr attr = {};
+ union bpf_prog_subtype *st = NULL;
+ struct bpf_subtype_data *sd = NULL;
+ struct bpf_load_program_attr load_attr;
attr.type = PERF_TYPE_TRACEPOINT;
attr.sample_type = PERF_SAMPLE_RAW;
@@ -120,6 +128,32 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
prog_type = BPF_PROG_TYPE_SK_SKB;
} else if (is_sk_msg) {
prog_type = BPF_PROG_TYPE_SK_MSG;
+ } else if (is_landlock) {
+ int i, prog_id;
+ const char *event_id = (event + 8);
+
+ if (!isdigit(*event_id)) {
+ printf("invalid prog number\n");
+ return -1;
+ }
+ prog_id = atoi(event_id);
+ for (i = 0; i < subtype_data_count; i++) {
+ if (subtype_data[i].name && strcmp(event,
+ subtype_data[i].name) == 0) {
+ /* save the prog_id for a next program */
+ sd = &subtype_data[i];
+ sd->prog_id = prog_id;
+ st = &sd->subtype;
+ free(sd->name);
+ sd->name = NULL;
+ break;
+ }
+ }
+ if (!st) {
+ printf("missing subtype\n");
+ return -1;
+ }
+ prog_type = BPF_PROG_TYPE_LANDLOCK_HOOK;
} else {
printf("Unknown event '%s'\n", event);
return -1;
@@ -128,16 +162,25 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
if (prog_cnt == MAX_PROGS)
return -1;
- fd = bpf_load_program(prog_type, prog, insns_cnt, license, kern_version,
- bpf_log_buf, BPF_LOG_BUF_SIZE);
+ memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
+ load_attr.prog_type = prog_type;
+ load_attr.prog_subtype = st;
+ load_attr.insns = prog;
+ load_attr.insns_cnt = insns_cnt;
+ load_attr.license = license;
+ load_attr.kern_version = kern_version;
+ fd = bpf_load_program_xattr(&load_attr, bpf_log_buf, BPF_LOG_BUF_SIZE);
if (fd < 0) {
printf("bpf_load_program() err=%d\n%s", errno, bpf_log_buf);
return -1;
}
+ if (sd)
+ sd->prog_fd = fd;
prog_fd[prog_cnt++] = fd;
- if (is_xdp || is_perf_event || is_cgroup_skb || is_cgroup_sk)
+ if (is_xdp || is_perf_event || is_cgroup_skb || is_cgroup_sk ||
+ is_landlock)
return 0;
if (is_socket || is_sockops || is_sk_skb || is_sk_msg) {
@@ -519,6 +562,7 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
kern_version = 0;
memset(license, 0, sizeof(license));
memset(processed_sec, 0, sizeof(processed_sec));
+ subtype_data_count = 0;
if (elf_version(EV_CURRENT) == EV_NONE)
return 1;
@@ -567,6 +611,29 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
data_maps = data;
for (j = 0; j < MAX_MAPS; j++)
map_data[j].fd = -1;
+ } else if (strncmp(shname, "subtype", 7) == 0) {
+ processed_sec[i] = true;
+ if (*(shname + 7) != '/') {
+ printf("invalid name of subtype section");
+ return 1;
+ }
+ if (data->d_size != sizeof(union bpf_prog_subtype)) {
+ printf("invalid size of subtype section: %zd\n",
+ data->d_size);
+ printf("ref: %zd\n",
+ sizeof(union bpf_prog_subtype));
+ return 1;
+ }
+ if (subtype_data_count >= MAX_PROGS) {
+ printf("too many subtype sections");
+ return 1;
+ }
+ memcpy(&subtype_data[subtype_data_count].subtype,
+ data->d_buf,
+ sizeof(union bpf_prog_subtype));
+ subtype_data[subtype_data_count].name =
+ strdup((shname + 8));
+ subtype_data_count++;
} else if (shdr.sh_type == SHT_SYMTAB) {
strtabidx = shdr.sh_link;
symbols = data;
@@ -643,7 +710,8 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
memcmp(shname, "cgroup/", 7) == 0 ||
memcmp(shname, "sockops", 7) == 0 ||
memcmp(shname, "sk_skb", 6) == 0 ||
- memcmp(shname, "sk_msg", 6) == 0) {
+ memcmp(shname, "sk_msg", 6) == 0 ||
+ memcmp(shname, "landlock", 8) == 0) {
ret = load_and_attach(shname, data->d_buf,
data->d_size);
if (ret != 0)
diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
index 814894a12974..e210b5fdf8ee 100644
--- a/samples/bpf/bpf_load.h
+++ b/samples/bpf/bpf_load.h
@@ -24,6 +24,13 @@ struct bpf_map_data {
struct bpf_load_map_def def;
};
+struct bpf_subtype_data {
+ char *name;
+ int prog_id;
+ int prog_fd;
+ union bpf_prog_subtype subtype;
+};
+
typedef void (*fixup_map_cb)(struct bpf_map_data *map, int idx);
extern int prog_fd[MAX_PROGS];
diff --git a/samples/bpf/landlock1.h b/samples/bpf/landlock1.h
new file mode 100644
index 000000000000..53b0a9447855
--- /dev/null
+++ b/samples/bpf/landlock1.h
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Landlock sample 1 - common header
+ *
+ * Copyright © 2018-2019 Mickaël Salaün <mic@digikod.net>
+ */
+
+#define MAP_FLAG_DENY (1ULL << 0)
diff --git a/samples/bpf/landlock1_kern.c b/samples/bpf/landlock1_kern.c
new file mode 100644
index 000000000000..0298d98dd06a
--- /dev/null
+++ b/samples/bpf/landlock1_kern.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Landlock sample 1 - whitelist of read only or read-write file hierarchy
+ *
+ * Copyright © 2017-2019 Mickaël Salaün <mic@digikod.net>
+ */
+
+/*
+ * This file contains a function that will be compiled to eBPF bytecode thanks
+ * to LLVM/Clang.
+ *
+ * Each SEC() means that the following function or variable will be part of a
+ * custom ELF section. This sections are then processed by the userspace part
+ * (see landlock1_user.c) to extract eBPF bytecode and take into account
+ * variables describing the eBPF program subtype or its license.
+ */
+
+#include <uapi/linux/bpf.h>
+#include <uapi/linux/landlock.h>
+
+#include "bpf_helpers.h"
+#include "landlock1.h" /* MAP_FLAG_DENY */
+
+#define MAP_MAX_ENTRIES 20
+
+SEC("maps")
+struct bpf_map_def inode_map = {
+ .type = BPF_MAP_TYPE_INODE,
+ .key_size = sizeof(u32),
+ .value_size = sizeof(u64),
+ .max_entries = MAP_MAX_ENTRIES,
+};
+
+static __always_inline __u64 get_access(void *inode)
+{
+ if (bpf_inode_map_lookup(&inode_map, inode) & MAP_FLAG_DENY)
+ return LANDLOCK_RET_DENY;
+ return LANDLOCK_RET_ALLOW;
+}
+
+SEC("subtype/landlock1")
+static union bpf_prog_subtype _subtype1 = {
+ .landlock_hook = {
+ .type = LANDLOCK_HOOK_FS_WALK,
+ }
+};
+
+/*
+ * The function fs_walk() is a simple Landlock program enforced on a set of
+ * processes. This program will be run for each walk through a file path.
+ *
+ * The argument ctx contains the context of the program when it is run, which
+ * enable to evaluate the file path. This context can change for each run of
+ * the program.
+ */
+SEC("landlock1")
+int fs_walk(struct landlock_ctx_fs_walk *ctx)
+{
+ return get_access((void *)ctx->inode);
+}
+
+SEC("subtype/landlock2")
+static union bpf_prog_subtype _subtype2 = {
+ .landlock_hook = {
+ .type = LANDLOCK_HOOK_FS_PICK,
+ /*
+ * allowed:
+ * - LANDLOCK_TRIGGER_FS_PICK_LINK
+ * - LANDLOCK_TRIGGER_FS_PICK_LINKTO
+ * - LANDLOCK_TRIGGER_FS_PICK_RECEIVE
+ * - LANDLOCK_TRIGGER_FS_PICK_MOUNTON
+ */
+ .triggers =
+ LANDLOCK_TRIGGER_FS_PICK_APPEND |
+ LANDLOCK_TRIGGER_FS_PICK_CHDIR |
+ LANDLOCK_TRIGGER_FS_PICK_CHROOT |
+ LANDLOCK_TRIGGER_FS_PICK_CREATE |
+ LANDLOCK_TRIGGER_FS_PICK_EXECUTE |
+ LANDLOCK_TRIGGER_FS_PICK_FCNTL |
+ LANDLOCK_TRIGGER_FS_PICK_GETATTR |
+ LANDLOCK_TRIGGER_FS_PICK_IOCTL |
+ LANDLOCK_TRIGGER_FS_PICK_LOCK |
+ LANDLOCK_TRIGGER_FS_PICK_MAP |
+ LANDLOCK_TRIGGER_FS_PICK_OPEN |
+ LANDLOCK_TRIGGER_FS_PICK_READ |
+ LANDLOCK_TRIGGER_FS_PICK_READDIR |
+ LANDLOCK_TRIGGER_FS_PICK_RENAME |
+ LANDLOCK_TRIGGER_FS_PICK_RENAMETO |
+ LANDLOCK_TRIGGER_FS_PICK_RMDIR |
+ LANDLOCK_TRIGGER_FS_PICK_SETATTR |
+ LANDLOCK_TRIGGER_FS_PICK_TRANSFER |
+ LANDLOCK_TRIGGER_FS_PICK_UNLINK |
+ LANDLOCK_TRIGGER_FS_PICK_WRITE,
+ }
+};
+
+SEC("landlock2")
+int fs_pick_ro(struct landlock_ctx_fs_pick *ctx)
+{
+ return get_access((void *)ctx->inode);
+}
+
+SEC("license")
+static const char _license[] = "GPL";
diff --git a/samples/bpf/landlock1_user.c b/samples/bpf/landlock1_user.c
new file mode 100644
index 000000000000..aa45932d36a8
--- /dev/null
+++ b/samples/bpf/landlock1_user.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Landlock sample 1 - deny access to a set of directories (blacklisting)
+ *
+ * Copyright © 2017-2019 Mickaël Salaün <mic@digikod.net>
+ */
+
+#include "bpf/libbpf.h"
+#include "bpf_load.h"
+#include "landlock1.h" /* MAP_FLAG_DENY */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h> /* open() */
+#include <linux/bpf.h>
+#include <linux/filter.h>
+#include <linux/landlock.h>
+#include <linux/prctl.h>
+#include <linux/seccomp.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+#ifndef seccomp
+static int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
+#endif
+
+static int apply_sandbox(int prog_fd)
+{
+ int ret = 0;
+
+ /* set up the test sandbox */
+ if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+ perror("prctl(no_new_priv)");
+ return 1;
+ }
+ if (seccomp(SECCOMP_PREPEND_LANDLOCK_PROG, 0, &prog_fd)) {
+ perror("seccomp(set_hook)");
+ ret = 1;
+ }
+ close(prog_fd);
+
+ return ret;
+}
+
+#define ENV_FS_PATH_DENY_NAME "LL_PATH_DENY"
+#define ENV_PATH_TOKEN ":"
+
+static int parse_path(char *env_path, const char ***path_list)
+{
+ int i, path_nb = 0;
+
+ if (env_path) {
+ path_nb++;
+ for (i = 0; env_path[i]; i++) {
+ if (env_path[i] == ENV_PATH_TOKEN[0])
+ path_nb++;
+ }
+ }
+ *path_list = malloc(path_nb * sizeof(**path_list));
+ for (i = 0; i < path_nb; i++)
+ (*path_list)[i] = strsep(&env_path, ENV_PATH_TOKEN);
+
+ return path_nb;
+}
+
+static int populate_map(const char *env_var, unsigned long long value,
+ int map_fd)
+{
+ int path_nb, ref_fd, i;
+ char *env_path_name;
+ const char **path_list = NULL;
+
+ env_path_name = getenv(env_var);
+ if (!env_path_name)
+ return 0;
+ env_path_name = strdup(env_path_name);
+ path_nb = parse_path(env_path_name, &path_list);
+
+ for (i = 0; i < path_nb; i++) {
+ ref_fd = open(path_list[i], O_RDONLY | O_CLOEXEC);
+ if (ref_fd < 0) {
+ fprintf(stderr, "Failed to open \"%s\": %s\n",
+ path_list[i],
+ strerror(errno));
+ return 1;
+ }
+ if (bpf_map_update_elem(map_fd, &ref_fd, &value, BPF_ANY)) {
+ fprintf(stderr, "Failed to update the map with"
+ " \"%s\": %s\n", path_list[i],
+ strerror(errno));
+ return 1;
+ }
+ close(ref_fd);
+ }
+ free(env_path_name);
+ return 0;
+}
+
+int main(int argc, char * const argv[], char * const *envp)
+{
+ char filename[256];
+ char *cmd_path;
+ char * const *cmd_argv;
+ int ll_prog_walk, ll_prog_pick;
+
+ if (argc < 2) {
+ fprintf(stderr, "usage: %s <cmd> [args]...\n\n", argv[0]);
+ fprintf(stderr, "Launch a command in a restricted environment.\n\n");
+ fprintf(stderr, "Environment variables containing paths, each separated by a colon:\n");
+ fprintf(stderr, "* %s: list of files and directories which are denied\n",
+ ENV_FS_PATH_DENY_NAME);
+ fprintf(stderr, "\nexample:\n"
+ "%s=\"${HOME}/.ssh:${HOME}/Images\" "
+ "%s /bin/sh -i\n",
+ ENV_FS_PATH_DENY_NAME, argv[0]);
+ return 1;
+ }
+
+ snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+ if (load_bpf_file(filename)) {
+ printf("%s", bpf_log_buf);
+ return 1;
+ }
+ ll_prog_walk = prog_fd[0]; /* fs_walk */
+ ll_prog_pick = prog_fd[1]; /* fs_pick */
+ if (!ll_prog_walk || !ll_prog_pick) {
+ if (errno)
+ printf("load_bpf_file: %s\n", strerror(errno));
+ else
+ printf("load_bpf_file: Error\n");
+ return 1;
+ }
+
+ if (populate_map(ENV_FS_PATH_DENY_NAME, MAP_FLAG_DENY, map_fd[0]))
+ return 1;
+ close(map_fd[0]);
+
+ fprintf(stderr, "Launching a new sandboxed process\n");
+ if (apply_sandbox(ll_prog_walk))
+ return 1;
+ if (apply_sandbox(ll_prog_pick))
+ return 1;
+ cmd_path = argv[1];
+ cmd_argv = argv + 1;
+ execve(cmd_path, cmd_argv, envp);
+ perror("Failed to call execve");
+ return 1;
+}
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next v9 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier
From: Mickaël Salaün @ 2019-06-25 21:52 UTC (permalink / raw)
To: linux-kernel
Cc: Mickaël Salaün, Aleksa Sarai, Alexander Viro,
Alexei Starovoitov, Andrew Morton, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Stephen Smalley,
Tejun Heo, Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <20190625215239.11136-1-mic@digikod.net>
The goal of the program subtype is to be able to have different static
fine-grained verifications for a unique program type.
The struct bpf_verifier_ops gets a new optional function:
is_valid_subtype(). This new verifier is called at the beginning of the
eBPF program verification to check if the (optional) program subtype is
valid.
The new helper bpf_load_program_xattr() enables to verify a program with
subtypes.
For now, only Landlock eBPF programs are using a program subtype (see
next commits) but this could be used by other program types in the
future.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
Link: https://lkml.kernel.org/r/20160827205559.GA43880@ast-mbp.thefacebook.com
---
Changes since v8:
* use bpf_load_program_xattr() instead of bpf_load_program() and add
bpf_verify_program_xattr() to deal with subtypes
* remove put_extra() since there is no more "previous" field (for now)
Changes since v7:
* rename LANDLOCK_SUBTYPE_* to LANDLOCK_*
* move subtype in bpf_prog_aux and use only one bit for has_subtype
(suggested by Alexei Starovoitov)
* wrap the prog_subtype with a prog_extra to be able to reference kernel
pointers:
* add an optional put_extra() function to struct bpf_prog_ops to be
able to free the pointed data
* replace all the prog_subtype with prog_extra in the struct
bpf_verifier_ops functions
* remove the ABI field (requested by Alexei Starovoitov)
* rename subtype fields
Changes since v6:
* rename Landlock version to ABI to better reflect its purpose
* fix unsigned integer checks
* fix pointer cast
* constify pointers
* rebase
Changes since v5:
* use a prog_subtype pointer and make it future-proof
* add subtype test
* constify bpf_load_program()'s subtype argument
* cleanup subtype initialization
* rebase
Changes since v4:
* replace the "status" field with "version" (more generic)
* replace the "access" field with "ability" (less confusing)
Changes since v3:
* remove the "origin" field
* add an "option" field
* cleanup comments
---
include/linux/bpf.h | 8 +++++
include/uapi/linux/bpf.h | 9 ++++++
kernel/bpf/core.c | 1 +
kernel/bpf/syscall.c | 32 ++++++++++++++++++-
kernel/bpf/verifier.c | 11 +++++++
net/core/filter.c | 25 ++++++++-------
tools/include/uapi/linux/bpf.h | 9 ++++++
tools/lib/bpf/bpf.c | 10 +++++-
tools/lib/bpf/bpf.h | 2 ++
tools/lib/bpf/libbpf.map | 1 +
tools/testing/selftests/bpf/test_verifier.c | 26 +++++++++++++--
.../testing/selftests/bpf/verifier/subtype.c | 10 ++++++
12 files changed, 128 insertions(+), 16 deletions(-)
create mode 100644 tools/testing/selftests/bpf/verifier/subtype.c
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a62e7889b0b6..da167d3afecc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -294,6 +294,11 @@ bpf_ctx_record_field_size(struct bpf_insn_access_aux *aux, u32 size)
aux->ctx_field_size = size;
}
+/* specific data per program type */
+struct bpf_prog_extra {
+ union bpf_prog_subtype subtype;
+};
+
struct bpf_prog_ops {
int (*test_run)(struct bpf_prog *prog, const union bpf_attr *kattr,
union bpf_attr __user *uattr);
@@ -319,6 +324,8 @@ struct bpf_verifier_ops {
const struct bpf_insn *src,
struct bpf_insn *dst,
struct bpf_prog *prog, u32 *target_size);
+ // TODO?: convert to bool (*is_valid_subtype)(struct bpf_prog *prog);
+ bool (*is_valid_subtype)(struct bpf_prog_extra *prog_extra);
};
struct bpf_prog_offload_ops {
@@ -418,6 +425,7 @@ struct bpf_prog_aux {
struct work_struct work;
struct rcu_head rcu;
};
+ struct bpf_prog_extra *extra;
};
struct bpf_array {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b077507efa3f..ddae50373d58 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -355,6 +355,13 @@ struct bpf_stack_build_id {
};
};
+union bpf_prog_subtype {
+ struct {
+ __u32 type; /* enum landlock_hook_type */
+ __aligned_u64 triggers; /* LANDLOCK_TRIGGER_* */
+ } landlock_hook;
+} __attribute__((aligned(8)));
+
union bpf_attr {
struct { /* anonymous struct used by BPF_MAP_CREATE command */
__u32 map_type; /* one of enum bpf_map_type */
@@ -409,6 +416,8 @@ union bpf_attr {
__u32 line_info_rec_size; /* userspace bpf_line_info size */
__aligned_u64 line_info; /* line info */
__u32 line_info_cnt; /* number of bpf_line_info records */
+ __aligned_u64 prog_subtype; /* bpf_prog_subtype address */
+ __u32 prog_subtype_size;
};
struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ad3be85f1411..8ad392e52328 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -255,6 +255,7 @@ void __bpf_prog_free(struct bpf_prog *fp)
{
if (fp->aux) {
free_percpu(fp->aux->stats);
+ kfree(fp->aux->extra);
kfree(fp->aux);
}
vfree(fp);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7713cf39795a..7dd3376904d4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1596,7 +1596,7 @@ bpf_prog_load_check_attach_type(enum bpf_prog_type prog_type,
}
/* last field in 'union bpf_attr' used by this command */
-#define BPF_PROG_LOAD_LAST_FIELD line_info_cnt
+#define BPF_PROG_LOAD_LAST_FIELD prog_subtype_size
static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
{
@@ -1686,6 +1686,36 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
if (err)
goto free_prog;
+ /* copy eBPF program subtype from user space */
+ if (attr->prog_subtype) {
+ u32 size;
+
+ err = bpf_check_uarg_tail_zero(
+ u64_to_user_ptr(attr->prog_subtype),
+ sizeof(prog->aux->extra->subtype),
+ attr->prog_subtype_size);
+ if (err)
+ goto free_prog;
+ size = min_t(u32, attr->prog_subtype_size,
+ sizeof(prog->aux->extra->subtype));
+
+ prog->aux->extra = kzalloc(sizeof(*prog->aux->extra),
+ GFP_KERNEL | GFP_USER);
+ if (!prog->aux->extra) {
+ err = -ENOMEM;
+ goto free_prog;
+ }
+ if (copy_from_user(&prog->aux->extra->subtype,
+ u64_to_user_ptr(attr->prog_subtype), size)
+ != 0) {
+ err = -EFAULT;
+ goto free_prog;
+ }
+ } else if (attr->prog_subtype_size != 0) {
+ err = -EINVAL;
+ goto free_prog;
+ }
+
/* run eBPF verifier */
err = bpf_check(&prog, attr, uattr);
if (err < 0)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0e079b2298f8..930260683d0a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9167,6 +9167,17 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
if (ret < 0)
goto skip_full_check;
+ if (env->ops->is_valid_subtype) {
+ if (!env->ops->is_valid_subtype(env->prog->aux->extra)) {
+ ret = -EINVAL;
+ goto err_unlock;
+ }
+ } else if (env->prog->aux->extra) {
+ /* do not accept a subtype if the program does not handle it */
+ ret = -EINVAL;
+ goto err_unlock;
+ }
+
if (bpf_prog_is_dev_bound(env->prog->aux)) {
ret = bpf_prog_offload_verifier_prep(env->prog);
if (ret)
diff --git a/net/core/filter.c b/net/core/filter.c
index 2014d76e0d2a..0160237e5cd8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5846,7 +5846,8 @@ bool bpf_helper_changes_pkt_data(void *func)
}
static const struct bpf_func_proto *
-bpf_base_func_proto(enum bpf_func_id func_id)
+bpf_base_func_proto(enum bpf_func_id func_id,
+ const struct bpf_prog_extra *prog_extra)
{
switch (func_id) {
case BPF_FUNC_map_lookup_elem:
@@ -5902,7 +5903,7 @@ sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_get_local_storage:
return &bpf_get_local_storage_proto;
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -5942,7 +5943,7 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_sk_storage_delete:
return &bpf_sk_storage_delete_proto;
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -5959,7 +5960,7 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_get_socket_uid:
return &bpf_get_socket_uid_proto;
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -6096,7 +6097,7 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_skb_ecn_set_ce_proto;
#endif
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -6135,7 +6136,7 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_tcp_check_syncookie_proto;
#endif
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -6171,7 +6172,7 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_tcp_sock_proto;
#endif /* CONFIG_INET */
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -6197,7 +6198,7 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_msg_pop_data:
return &bpf_msg_pop_data_proto;
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -6237,7 +6238,7 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_skc_lookup_tcp_proto;
#endif
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -6248,7 +6249,7 @@ flow_dissector_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_skb_load_bytes:
return &bpf_flow_dissector_load_bytes_proto;
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -6275,7 +6276,7 @@ lwt_out_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_skb_under_cgroup:
return &bpf_skb_under_cgroup_proto;
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
@@ -8611,7 +8612,7 @@ sk_reuseport_func_proto(enum bpf_func_id func_id,
case BPF_FUNC_skb_load_bytes_relative:
return &sk_reuseport_load_bytes_relative_proto;
default:
- return bpf_base_func_proto(func_id);
+ return bpf_base_func_proto(func_id, prog->aux->extra);
}
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b077507efa3f..ddae50373d58 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -355,6 +355,13 @@ struct bpf_stack_build_id {
};
};
+union bpf_prog_subtype {
+ struct {
+ __u32 type; /* enum landlock_hook_type */
+ __aligned_u64 triggers; /* LANDLOCK_TRIGGER_* */
+ } landlock_hook;
+} __attribute__((aligned(8)));
+
union bpf_attr {
struct { /* anonymous struct used by BPF_MAP_CREATE command */
__u32 map_type; /* one of enum bpf_map_type */
@@ -409,6 +416,8 @@ union bpf_attr {
__u32 line_info_rec_size; /* userspace bpf_line_info size */
__aligned_u64 line_info; /* line info */
__u32 line_info_cnt; /* number of bpf_line_info records */
+ __aligned_u64 prog_subtype; /* bpf_prog_subtype address */
+ __u32 prog_subtype_size;
};
struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index c7d7993c44bb..ecd894344bbd 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -227,6 +227,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
memset(&attr, 0, sizeof(attr));
attr.prog_type = load_attr->prog_type;
+ attr.prog_subtype = ptr_to_u64(load_attr->prog_subtype);
+ attr.prog_subtype_size = load_attr->prog_subtype ?
+ sizeof(*load_attr->prog_subtype) : 0;
attr.expected_attach_type = load_attr->expected_attach_type;
attr.insn_cnt = (__u32)load_attr->insns_cnt;
attr.insns = ptr_to_u64(load_attr->insns);
@@ -332,6 +335,11 @@ int bpf_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
return bpf_load_program_xattr(&load_attr, log_buf, log_buf_sz);
}
+int bpf_verify_program_xattr(union bpf_attr *attr, size_t attr_sz)
+{
+ return sys_bpf_prog_load(attr, attr_sz);
+}
+
int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns,
size_t insns_cnt, __u32 prog_flags, const char *license,
__u32 kern_version, char *log_buf, size_t log_buf_sz,
@@ -351,7 +359,7 @@ int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns,
attr.kern_version = kern_version;
attr.prog_flags = prog_flags;
- return sys_bpf_prog_load(&attr, sizeof(attr));
+ return bpf_verify_program_xattr(&attr, sizeof(attr));
}
int bpf_map_update_elem(int fd, const void *key, const void *value,
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index ff42ca043dc8..7cd600d49f9e 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -88,6 +88,7 @@ struct bpf_load_program_attr {
__u32 line_info_cnt;
__u32 log_level;
__u32 prog_flags;
+ const union bpf_prog_subtype *prog_subtype;
};
/* Flags to direct loading requirements */
@@ -102,6 +103,7 @@ LIBBPF_API int bpf_load_program(enum bpf_prog_type type,
const struct bpf_insn *insns, size_t insns_cnt,
const char *license, __u32 kern_version,
char *log_buf, size_t log_buf_sz);
+LIBBPF_API int bpf_verify_program_xattr(union bpf_attr *attr, size_t attr_sz);
LIBBPF_API int bpf_verify_program(enum bpf_prog_type type,
const struct bpf_insn *insns,
size_t insns_cnt, __u32 prog_flags,
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 2c6d835620d2..48c9269a0496 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -107,6 +107,7 @@ LIBBPF_0.0.1 {
bpf_set_link_xdp_fd;
bpf_task_fd_query;
bpf_verify_program;
+ bpf_verify_program_xattr;
btf__fd;
btf__find_by_name;
btf__free;
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index c5514daf8865..93faffd31fc3 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -105,6 +105,8 @@ struct bpf_test {
__u64 data64[TEST_DATA_LEN / 8];
};
} retvals[MAX_TEST_RUNS];
+ bool has_prog_subtype;
+ union bpf_prog_subtype prog_subtype;
};
/* Note we want this to be 64 bit aligned so that the end of our array is
@@ -122,6 +124,11 @@ struct other_val {
long long bar;
};
+static inline __u64 ptr_to_u64(const void *ptr)
+{
+ return (__u64) (unsigned long) ptr;
+}
+
static void bpf_fill_ld_abs_vlan_push_pop(struct bpf_test *self)
{
/* test: {skb->data[0], vlan_push} x 51 + {skb->data[0], vlan_pop} x 51 */
@@ -856,6 +863,9 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
int fixup_skips;
__u32 pflags;
int i, err;
+ union bpf_attr attr;
+ union bpf_prog_subtype *prog_subtype =
+ test->has_prog_subtype ? &test->prog_subtype : NULL;
for (i = 0; i < MAX_NR_MAPS; i++)
map_fds[i] = -1;
@@ -881,8 +891,20 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
pflags |= BPF_F_STRICT_ALIGNMENT;
if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS)
pflags |= BPF_F_ANY_ALIGNMENT;
- fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
- "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
+
+ memset(&attr, 0, sizeof(attr));
+ attr.prog_type = prog_type;
+ attr.prog_subtype = ptr_to_u64(prog_subtype);
+ attr.prog_subtype_size = prog_subtype ? sizeof(*prog_subtype) : 0;
+ attr.insn_cnt = (__u32)prog_len;
+ attr.insns = ptr_to_u64(prog);
+ attr.license = ptr_to_u64("GPL");
+ bpf_vlog[0] = 0;
+ attr.log_buf = ptr_to_u64(bpf_vlog);
+ attr.log_size = sizeof(bpf_vlog);
+ attr.log_level = 4;
+ attr.prog_flags = pflags;
+ fd_prog = bpf_verify_program_xattr(&attr, sizeof(attr));
if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
printf("SKIP (unsupported program type %d)\n", prog_type);
skips++;
diff --git a/tools/testing/selftests/bpf/verifier/subtype.c b/tools/testing/selftests/bpf/verifier/subtype.c
new file mode 100644
index 000000000000..cf614223d53f
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/subtype.c
@@ -0,0 +1,10 @@
+{
+ "superfluous subtype",
+ .insns = {
+ BPF_MOV32_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "",
+ .result = REJECT,
+ .has_prog_subtype = true,
+},
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v6 0/8] net/sched: Add txtime-assist support for taprio.
From: Vedang Patel @ 2019-06-25 22:07 UTC (permalink / raw)
To: netdev
Cc: jeffrey.t.kirsher, davem, jhs, xiyou.wangcong, jiri,
intel-wired-lan, vinicius.gomes, l, jakub.kicinski, m-karicheri2,
sergei.shtylyov, eric.dumazet, aaron.f.brown, Vedang Patel
Changes in v6:
- Use _BITUL() instead of BIT() in UAPI for etf. (patch #1)
- Fix a bug reported by kbuild test bot in length_to_duration(). (patch #6)
- Remove an unused function (get_cycle_start()). (Patch #6)
Changes in v5:
- Commit message improved for the igb patch (patch #1).
- Fixed typo in commit message for etf patch (patch #2).
Changes in v4:
- Remove inline directive from functions in foo.c.
- Fix spacing in pkt_sched.h (for etf patch).
Changes in v3:
- Simplify implementation for taprio flags.
- txtime_delay can only be set if txtime-assist mode is enabled.
- txtime_delay and flags will only be visible in tc output if set by user.
- Minor changes in error reporting.
Changes in v2:
- Txtime-offload has now been renamed to txtime-assist mode.
- Renamed the offload parameter to flags.
- Removed the code which introduced the hardware offloading functionality.
Original Cover letter (with above changes included)
--------------------------------------------------
Currently, we are seeing packets being transmitted outside their
timeslices. We can confirm that the packets are being dequeued at the right
time. So, the delay is induced after the packet is dequeued, because
taprio, without any offloading, has no control of when a packet is actually
transmitted.
In order to solve this, we are making use of the txtime feature provided by
ETF qdisc. Hardware offloading needs to be supported by the ETF qdisc in
order to take advantage of this feature. The taprio qdisc will assign
txtime (in skb->tstamp) for all the packets which do not have the txtime
allocated via the SO_TXTIME socket option. For the packets which already
have SO_TXTIME set, taprio will validate whether the packet will be
transmitted in the correct interval.
In order to support this, the following parameters have been added:
- flags (taprio): This is added in order to support different offloading
modes which will be added in the future.
- txtime-delay (taprio): This indicates the minimum time it will take for
the packet to hit the wire after it reaches taprio_enqueue(). This is
useful in determining whether we can transmit the packet in the remaining
time if the gate corresponding to the packet is currently open.
- skip_skb_check (ETF): ETF currently drops any packet which does not have
the SO_TXTIME socket option set. This check can be skipped by specifying
this option.
Following is an example configuration:
tc qdisc replace dev $IFACE parent root handle 100 taprio \\
num_tc 3 \\
map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \\
queues 1@0 1@0 1@0 \\
base-time $BASE_TIME \\
sched-entry S 01 300000 \\
sched-entry S 02 300000 \\
sched-entry S 04 400000 \\
flags 0x1 \\
txtime-delay 200000 \\
clockid CLOCK_TAI
tc qdisc replace dev $IFACE parent 100:1 etf \\
offload delta 200000 clockid CLOCK_TAI skip_skb_check
Here, the "flags" parameter is indicating that the txtime-assist mode is
enabled. Also, all the traffic classes have been assigned the same queue.
This is to prevent the traffic classes in the lower priority queues from
getting starved. Note that this configuration is specific to the i210
ethernet card. Other network cards where the hardware queues are given the
same priority, might be able to utilize more than one queue.
Following are some of the other highlights of the series:
- Fix a bug where hardware timestamping and SO_TXTIME options cannot be
used together. (Patch 1)
- Introduces the skip_skb_check option. (Patch 2)
- Make TxTime assist mode work with TCP packets (Patch 7).
The following changes are recommended to be done in order to get the best
performance from taprio in this mode:
# TSN in general does not allow Jumbo frames.
ip link set dev enp1s0 mtu 1514
# Disable segmentation offload. This is to prevent NIC from sending packets
# after the gate for a traffic class has closed.
ethtool -K eth0 gso off
ethtool -K eth0 tso off
# Disable energy efficient ethernet to make sure there are no latency
# spikes when NIC is trying to wake up when the packet is supposed to be
# sent.
ethtool --set-eee eth0 eee off
Thanks,
Vedang Patel
Vedang Patel (8):
igb: clear out skb->tstamp after reading the txtime
etf: Don't use BIT() in UAPI headers.
etf: Add skip_sock_check
taprio: calculate cycle_time when schedule is installed
taprio: Remove inline directive
taprio: Add support for txtime-assist mode
taprio: make clock reference conversions easier
taprio: Adjust timestamps for TCP packets
drivers/net/ethernet/intel/igb/igb_main.c | 1 +
include/uapi/linux/pkt_sched.h | 9 +-
net/sched/sch_etf.c | 10 +
net/sched/sch_taprio.c | 421 +++++++++++++++++++++++++++---
4 files changed, 405 insertions(+), 36 deletions(-)
--
2.7.3
^ 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