* [PATCH v12 8/8] mm: Don't split THP page when syscall is called
From: Minchan Kim @ 2014-07-09 6:22 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-mm, Michael Kerrisk, Linux API, Hugh Dickins,
Johannes Weiner, Rik van Riel, KOSAKI Motohiro, Mel Gorman,
Jason Evans, Zhang Yanfei, Kirill A. Shutemov, Minchan Kim,
Andrea Arcangeli
In-Reply-To: <1404886949-17695-1-git-send-email-minchan@kernel.org>
We don't need to split THP page when MADV_FREE syscall is
called. It could be done when VM decide really frees it so
we could avoid unnecessary THP split.
Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/huge_mm.h | 4 ++++
mm/huge_memory.c | 35 +++++++++++++++++++++++++++++++++++
mm/madvise.c | 21 ++++++++++++++++++++-
mm/rmap.c | 8 ++++++--
mm/vmscan.c | 28 ++++++++++++++++++----------
5 files changed, 83 insertions(+), 13 deletions(-)
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 63579cb8d3dc..25a961256d9f 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -19,6 +19,9 @@ extern struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
unsigned long addr,
pmd_t *pmd,
unsigned int flags);
+extern int madvise_free_huge_pmd(struct mmu_gather *tlb,
+ struct vm_area_struct *vma,
+ pmd_t *pmd, unsigned long addr);
extern int zap_huge_pmd(struct mmu_gather *tlb,
struct vm_area_struct *vma,
pmd_t *pmd, unsigned long addr);
@@ -56,6 +59,7 @@ extern pmd_t *page_check_address_pmd(struct page *page,
unsigned long address,
enum page_check_address_pmd_flag flag,
spinlock_t **ptl);
+extern int pmd_freeable(pmd_t pmd);
#define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT)
#define HPAGE_PMD_NR (1<<HPAGE_PMD_ORDER)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 5d562a9fe931..919c780015b8 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1384,6 +1384,36 @@ out:
return 0;
}
+int madvise_free_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
+ pmd_t *pmd, unsigned long addr)
+
+{
+ spinlock_t *ptl;
+ struct mm_struct *mm = tlb->mm;
+ int ret = 1;
+
+ if (pmd_trans_huge_lock(pmd, vma, &ptl) == 1) {
+ struct page *page;
+ pmd_t orig_pmd;
+
+ orig_pmd = pmdp_get_and_clear(mm, addr, pmd);
+
+ /* No hugepage in swapcache */
+ page = pmd_page(orig_pmd);
+ VM_BUG_ON_PAGE(PageSwapCache(page), page);
+
+ orig_pmd = pmd_mkold(orig_pmd);
+ orig_pmd = pmd_mkclean(orig_pmd);
+
+ set_pmd_at(mm, addr, pmd, orig_pmd);
+ tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
+ spin_unlock(ptl);
+ ret = 0;
+ }
+
+ return ret;
+}
+
int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
pmd_t *pmd, unsigned long addr)
{
@@ -1620,6 +1650,11 @@ unlock:
return NULL;
}
+int pmd_freeable(pmd_t pmd)
+{
+ return !pmd_dirty(pmd);
+}
+
static int __split_huge_page_splitting(struct page *page,
struct vm_area_struct *vma,
unsigned long address)
diff --git a/mm/madvise.c b/mm/madvise.c
index 55b42e5e32a3..0927eba6d858 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -271,8 +271,26 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
spinlock_t *ptl;
pte_t *pte, ptent;
struct page *page;
+ unsigned long next;
+
+ next = pmd_addr_end(addr, end);
+ if (pmd_trans_huge(*pmd)) {
+ if (next - addr != HPAGE_PMD_SIZE) {
+#ifdef CONFIG_DEBUG_VM
+ if (!rwsem_is_locked(&mm->mmap_sem)) {
+ pr_err("%s: mmap_sem is unlocked! addr=0x%lx end=0x%lx vma->vm_start=0x%lx vma->vm_end=0x%lx\n",
+ __func__, addr, end,
+ vma->vm_start,
+ vma->vm_end);
+ BUG();
+ }
+#endif
+ split_huge_page_pmd(vma, addr, pmd);
+ } else if (!madvise_free_huge_pmd(tlb, vma, pmd, addr))
+ goto next;
+ /* fall through */
+ }
- split_huge_page_pmd(vma, addr, pmd);
if (pmd_trans_unstable(pmd))
return 0;
@@ -312,6 +330,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
}
arch_leave_lazy_mmu_mode();
pte_unmap_unlock(pte - 1, ptl);
+next:
cond_resched();
return 0;
}
diff --git a/mm/rmap.c b/mm/rmap.c
index 7ac6e059aba7..5a0cca9bc263 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -704,9 +704,13 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
referenced++;
/*
- * In this implmentation, MADV_FREE doesn't support THP free
+ * Use pmd_freeable instead of raw pmd_dirty because in some
+ * of architecture, pmd_dirty is not defined unless
+ * CONFIG_TRANSPARNTE_HUGE is enabled
*/
- dirty++;
+ if (!pmd_freeable(*pmd))
+ dirty++;
+
spin_unlock(ptl);
} else {
pte_t *pte;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 914815a43362..995b28e3aee7 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -971,17 +971,25 @@ static unsigned long shrink_page_list(struct list_head *page_list,
* Anonymous process memory has backing store?
* Try to allocate it some swap space here.
*/
- if (PageAnon(page) && !PageSwapCache(page) && !freeable) {
- if (!(sc->gfp_mask & __GFP_IO))
- goto keep_locked;
- if (!add_to_swap(page, page_list))
- goto activate_locked;
- may_enter_fs = 1;
-
- /* Adding to swap updated mapping */
- mapping = page_mapping(page);
+ if (PageAnon(page) && !PageSwapCache(page)) {
+ if (!freeable) {
+ if (!(sc->gfp_mask & __GFP_IO))
+ goto keep_locked;
+ if (!add_to_swap(page, page_list))
+ goto activate_locked;
+ may_enter_fs = 1;
+ /* Adding to swap updated mapping */
+ mapping = page_mapping(page);
+ } else {
+ if (likely(!PageTransHuge(page)))
+ goto unmap;
+ /* try_to_unmap isn't aware of THP page */
+ if (unlikely(split_huge_page_to_list(page,
+ page_list)))
+ goto keep_locked;
+ }
}
-
+unmap:
/*
* The page is mapped into the page tables of one or more
* processes. Try to unmap it here.
--
2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* [net-next PATCH v2 0/3] Broadcast/Multicast rate limit via Ethtool Coalesce
From: Mugunthan V N @ 2014-07-09 7:14 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N
A system/cpu can be loaded by a hacker with flooding of broadcast or
multicast packets, to prevent this some Ethernet controllers like CPSW
provide a mechanism to limit the broadcast/multicast packet rate via
hardware limiters. This patch series enables this feature via
Ethtool Coalesce.
Mugunthan V N (3):
net: ethtool: Add Multicast and broadcast rate limit coalescing
feature
drivers: net: cpsw: remove redundancy check
drivers: net: cpsw: Add support for multicast/broadcast rate limit
drivers/net/ethernet/ti/cpsw.c | 83 +++++++++++++++++++++++++++++++++++++++---
include/uapi/linux/ethtool.h | 4 ++
2 files changed, 82 insertions(+), 5 deletions(-)
--
2.0.0.390.gcb682f8
^ permalink raw reply
* [net-next PATCH v2 1/3] net: ethtool: Add Multicast and broadcast rate limit coalescing feature
From: Mugunthan V N @ 2014-07-09 7:14 UTC (permalink / raw)
To: netdev; +Cc: davem, linux-api, linux-kernel, Mugunthan V N
In-Reply-To: <1404890050-4020-1-git-send-email-mugunthanvnm@ti.com>
Add ability to limit the broadcast or multicast packets to prevent
the loading of the CPU by a hacker with a broadcast or multicast
packet flood.
Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
include/uapi/linux/ethtool.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index e3c7a71..96ade34 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -350,6 +350,8 @@ struct ethtool_modinfo {
* a TX interrupt, when the packet rate is above @pkt_rate_high.
* @rate_sample_interval: How often to do adaptive coalescing packet rate
* sampling, measured in seconds. Must not be zero.
+ * @rx_max_mcast: Threshold for high multicast packet rate (packets per second)
+ * @rx_max_bcast: Threshold for high broadcast packet rate (packets per second)
*
* Each pair of (usecs, max_frames) fields specifies that interrupts
* should be coalesced until
@@ -400,6 +402,8 @@ struct ethtool_coalesce {
__u32 tx_coalesce_usecs_high;
__u32 tx_max_coalesced_frames_high;
__u32 rate_sample_interval;
+ __u32 rx_max_mcast;
+ __u32 rx_max_bcast;
};
/**
--
2.0.0.390.gcb682f8
^ permalink raw reply related
* [net-next PATCH v2 2/3] drivers: net: cpsw: remove redundancy check
From: Mugunthan V N @ 2014-07-09 7:14 UTC (permalink / raw)
To: netdev; +Cc: davem, linux-api, linux-kernel, Mugunthan V N
In-Reply-To: <1404890050-4020-1-git-send-email-mugunthanvnm@ti.com>
In cpsw_set_coalesce, rx_coalesce_usecs is already checked before
calling this function, so removing redundancy check.
Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index b988d16..a6117e6 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -884,9 +884,6 @@ static int cpsw_set_coalesce(struct net_device *ndev,
u32 addnl_dvdr = 1;
u32 coal_intvl = 0;
- if (!coal->rx_coalesce_usecs)
- return -EINVAL;
-
coal_intvl = coal->rx_coalesce_usecs;
int_ctrl = readl(&priv->wr_regs->int_control);
--
2.0.0.390.gcb682f8
^ permalink raw reply related
* [net-next PATCH v2 3/3] drivers: net: cpsw: Add support for multicast/broadcast rate limit
From: Mugunthan V N @ 2014-07-09 7:14 UTC (permalink / raw)
To: netdev; +Cc: davem, linux-api, linux-kernel, Mugunthan V N
In-Reply-To: <1404890050-4020-1-git-send-email-mugunthanvnm@ti.com>
Add support for multicast/broadcast rate limit feature via ethtool coalesce.
Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 80 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 78 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index a6117e6..2a984e6 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -403,6 +403,8 @@ struct cpsw_priv {
bool irq_enabled;
struct cpts *cpts;
u32 emac_port;
+ u32 rx_max_mcast;
+ u32 rx_max_bcast;
};
struct cpsw_stats {
@@ -871,11 +873,13 @@ static int cpsw_get_coalesce(struct net_device *ndev,
struct cpsw_priv *priv = netdev_priv(ndev);
coal->rx_coalesce_usecs = priv->coal_intvl;
+ coal->rx_max_mcast = priv->rx_max_mcast;
+ coal->rx_max_bcast = priv->rx_max_bcast;
return 0;
}
-static int cpsw_set_coalesce(struct net_device *ndev,
- struct ethtool_coalesce *coal)
+static int cpsw_set_coalesce_usecs(struct net_device *ndev,
+ struct ethtool_coalesce *coal)
{
struct cpsw_priv *priv = netdev_priv(ndev);
u32 int_ctrl;
@@ -933,6 +937,74 @@ static int cpsw_set_coalesce(struct net_device *ndev,
return 0;
}
+static int cpsw_set_coalesce_mcast(struct net_device *ndev,
+ struct ethtool_coalesce *coal)
+{
+ struct cpsw_priv *priv = netdev_priv(ndev);
+ int port;
+
+ priv->rx_max_mcast = coal->rx_max_mcast;
+
+ if (priv->data.dual_emac)
+ port = priv->emac_port;
+ else
+ port = priv->data.active_slave;
+
+ cpsw_ale_control_set(priv->ale, port, ALE_PORT_MCAST_LIMIT,
+ coal->rx_max_mcast);
+
+ dev_dbg(priv->dev, "rx_max_mcast set to %d\n", priv->rx_max_mcast);
+ return 0;
+}
+
+static int cpsw_set_coalesce_bcast(struct net_device *ndev,
+ struct ethtool_coalesce *coal)
+{
+ struct cpsw_priv *priv = netdev_priv(ndev);
+ int port;
+
+ priv->rx_max_bcast = coal->rx_max_bcast;
+
+ if (priv->data.dual_emac)
+ port = priv->emac_port + 1;
+ else
+ port = priv->data.active_slave + 1;
+
+ cpsw_ale_control_set(priv->ale, port, ALE_PORT_BCAST_LIMIT,
+ coal->rx_max_bcast);
+
+ dev_dbg(priv->dev, "rx_max_mcast set to %d\n", priv->rx_max_bcast);
+ return 0;
+}
+
+static int cpsw_set_coalesce(struct net_device *ndev,
+ struct ethtool_coalesce *coal)
+{
+ int ret = -EINVAL;
+
+ if (coal->rx_coalesce_usecs) {
+ ret = cpsw_set_coalesce_usecs(ndev, coal);
+ if (ret) {
+ dev_err(&ndev->dev, "set rx-usecs failed\n");
+ return ret;
+ }
+ }
+
+ ret = cpsw_set_coalesce_mcast(ndev, coal);
+ if (ret) {
+ dev_err(&ndev->dev, "set coalesce rx-max-mcast failed\n");
+ return ret;
+ }
+
+ ret = cpsw_set_coalesce_bcast(ndev, coal);
+ if (ret) {
+ dev_err(&ndev->dev, "set coalesce rx-max-bcast failed\n");
+ return ret;
+ }
+
+ return ret;
+}
+
static int cpsw_get_sset_count(struct net_device *ndev, int sset)
{
switch (sset) {
@@ -1227,6 +1299,10 @@ static int cpsw_ndo_open(struct net_device *ndev)
/* enable statistics collection only on all ports */
__raw_writel(0x7, &priv->regs->stat_port_en);
+ /* Enable rate limit feature in the switch for rx only */
+ cpsw_ale_control_set(priv->ale, 0, ALE_RATE_LIMIT, 1);
+ cpsw_ale_control_set(priv->ale, 0, ALE_RATE_LIMIT_TX, 0);
+
if (WARN_ON(!priv->data.rx_descs))
priv->data.rx_descs = 128;
--
2.0.0.390.gcb682f8
^ permalink raw reply related
* Re: [PATCH 01/11] fs: add O_BENEATH_ONLY flag to openat(2)
From: Christoph Hellwig @ 2014-07-09 8:48 UTC (permalink / raw)
To: David Drysdale
Cc: Christoph Hellwig, LSM List, linux-kernel@vger.kernel.org,
Greg Kroah-Hartman, Alexander Viro, Meredydd Luff, Kees Cook,
James Morris, Linux API
In-Reply-To: <CAHse=S-gBbYe65mo6tBSJ2p=Z5aswgJTFvt9r29aHP3ALfJ0Mw@mail.gmail.com>
On Tue, Jul 08, 2014 at 05:54:24PM +0100, David Drysdale wrote:
> > How is this implemented in FreeBSD? I can't find any references to
> > O_BENEATH_ONLY except for your patchset.
>
> FreeBSD have the relative-only behaviour for openat() relative to a
> Capsicum capability dfd [1], and for a process in capability-mode [2],
> but they don't have the O_BENEATH_ONLY as a separately-accessible
> openat() flag. However, it seemed like a more widely useful idea so
> separating it out was suggested.
In that case we should make sure to use the same name and semantics for
it. As far as I'm concerned I'd prefer a less clumsy name like
O_BENEATH.
^ permalink raw reply
* Re: [PATCH v3 0/3] mm: introduce fincore() v3
From: Christoph Hellwig @ 2014-07-09 8:51 UTC (permalink / raw)
To: Naoya Horiguchi
Cc: Christoph Hellwig, Andrew Morton, Konstantin Khlebnikov,
Wu Fengguang, Arnaldo Carvalho de Melo, Borislav Petkov,
Kirill A. Shutemov, Johannes Weiner, Rusty Russell, David Miller,
Andres Freund, linux-kernel, linux-mm, Dave Hansen, Dave Chinner,
Michael Kerrisk, Linux API, Naoya Horiguchi
In-Reply-To: <20140708132755.GA24698@nhori>
On Tue, Jul 08, 2014 at 09:27:55AM -0400, Naoya Horiguchi wrote:
> On Tue, Jul 08, 2014 at 05:16:18AM -0700, Christoph Hellwig wrote:
> > Still a hard NAK for exposing page flags in a syscall ABI. These are
> > way to volatile to go into an application interface.
>
> Is there any specific reason that exporting via syscall ABI is more
> volatile than exporting via procfs as /proc/kpageflags alreadly does?
An optional proc debug output is very different from an actual system
call.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 0/7] File Sealing & memfd_create()
From: Hugh Dickins @ 2014-07-09 8:53 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Andrew Morton, Hugh Dickins, Linus Torvalds,
Michael Kerrisk, Ryan Lortie, linux-mm, linux-fsdevel, Linux API,
Greg Kroah-Hartman, John Stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Tony Battersby, Andy Lutomirski
In-Reply-To: <CANq1E4QZ95RmJ7i=6TzEP4e+WREzKtXmmjjDrvz4BgAhVHoeuQ@mail.gmail.com>
On Tue, 8 Jul 2014, David Herrmann wrote:
>
> Hugh, any comments on patch 5, 6 and 7? Those are the last outstanding
> issues with memfd+sealing. Patch 7 (isolating pages) is still my
> favorite and has been running just fine on my machine for the last
> months. I think it'd be nice if we could give it a try in -next. We
> can always fall back to Patch 5 or Patch 5+6. Those will detect any
> racing AIO and just fail or wait for the IO to finish for a short
> period.
It's distressing for both of us how slow I am to review these, sorry.
We have just too many bugs in mm (and yes, some of them mine) for me
to set aside time to get deep enough into new features.
I've been trying for days and weeks to get there, made some progress
today, and hope to continue tomorrow. I'll send my comments on 1/7
(thumb up) and 7/7 (thumb down) in a moment: 2-6 not tonight.
>
> Are there any other blockers for this?
Trivia only, I haven't noticed any blocker; though I'm still not quite
convinced by memfd_create() - but happy enough with it if others are.
Hugh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 1/7] mm: allow drivers to prevent new writable mappings
From: Hugh Dickins @ 2014-07-09 8:55 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-2-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> The i_mmap_writable field counts existing writable mappings of an
> address_space. To allow drivers to prevent new writable mappings, make
> this counter signed and prevent new writable mappings if it is negative.
> This is modelled after i_writecount and DENYWRITE.
>
> This will be required by the shmem-sealing infrastructure to prevent any
> new writable mappings after the WRITE seal has been set. In case there
> exists a writable mapping, this operation will fail with EBUSY.
>
> Note that we rely on the fact that iff you already own a writable mapping,
> you can increase the counter without using the helpers. This is the same
> that we do for i_writecount.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
I'm very pleased with the way you chose to do this in the end, following
the way VM_DENYWRITE does it: it works out nicer than I had imagined.
I do have some comments below, but the only one where I end up asking
for a change is to remove the void "return "; but please read through
in case any of my wonderings lead you to change your mind on something.
I am very tempted to suggest that you add VM_WRITE into those VM_SHARED
tests, and put the then necessary additional tests into mprotect.c: so
that you would no longer have the odd case that a VM_SHARED !VM_WRITE
area has to be unmapped before sealing the fd.
But that would involve an audit of flush_dcache_page() architectures,
and perhaps some mods to keep them safe across the mprotect(): let's
put it down as a desirable future enhancement, just to remove an odd
restriction.
With the "return " gone,
Acked-by: Hugh Dickins <hughd@google.com>
> ---
> fs/inode.c | 1 +
> include/linux/fs.h | 29 +++++++++++++++++++++++++++--
> kernel/fork.c | 2 +-
> mm/mmap.c | 24 ++++++++++++++++++------
> mm/swap_state.c | 1 +
> 5 files changed, 48 insertions(+), 9 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 6eecb7f..9945b11 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -165,6 +165,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
> mapping->a_ops = &empty_aops;
> mapping->host = inode;
> mapping->flags = 0;
> + atomic_set(&mapping->i_mmap_writable, 0);
> mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
> mapping->private_data = NULL;
> mapping->backing_dev_info = &default_backing_dev_info;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 338e6f7..71d17c9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -387,7 +387,7 @@ struct address_space {
> struct inode *host; /* owner: inode, block_device */
> struct radix_tree_root page_tree; /* radix tree of all pages */
> spinlock_t tree_lock; /* and lock protecting it */
> - unsigned int i_mmap_writable;/* count VM_SHARED mappings */
> + atomic_t i_mmap_writable;/* count VM_SHARED mappings */
> struct rb_root i_mmap; /* tree of private and shared mappings */
> struct list_head i_mmap_nonlinear;/*list VM_NONLINEAR mappings */
> struct mutex i_mmap_mutex; /* protect tree, count, list */
> @@ -470,10 +470,35 @@ static inline int mapping_mapped(struct address_space *mapping)
> * Note that i_mmap_writable counts all VM_SHARED vmas: do_mmap_pgoff
> * marks vma as VM_SHARED if it is shared, and the file was opened for
> * writing i.e. vma may be mprotected writable even if now readonly.
> + *
> + * If i_mmap_writable is negative, no new writable mappings are allowed. You
> + * can only deny writable mappings, if none exists right now.
> */
> static inline int mapping_writably_mapped(struct address_space *mapping)
> {
> - return mapping->i_mmap_writable != 0;
> + return atomic_read(&mapping->i_mmap_writable) > 0;
I have a slight anxiety that you're halving the writable range here:
but I think that if we can overflow with this change, we could overflow
before; and there are int counts elsewhere which would overflow easier.
> +}
> +
> +static inline int mapping_map_writable(struct address_space *mapping)
> +{
> + return atomic_inc_unless_negative(&mapping->i_mmap_writable) ?
> + 0 : -EPERM;
> +}
> +
> +static inline void mapping_unmap_writable(struct address_space *mapping)
> +{
> + return atomic_dec(&mapping->i_mmap_writable);
Better just
atomic_dec(&mapping->i_mmap_writable);
I didn't realize before that the compiler allows you to return a void
function from a void function without warning, but better not rely on that.
> +}
> +
> +static inline int mapping_deny_writable(struct address_space *mapping)
> +{
> + return atomic_dec_unless_positive(&mapping->i_mmap_writable) ?
> + 0 : -EBUSY;
> +}
> +
> +static inline void mapping_allow_writable(struct address_space *mapping)
> +{
> + atomic_inc(&mapping->i_mmap_writable);
> }
>
> /*
> diff --git a/kernel/fork.c b/kernel/fork.c
> index d2799d1..f1f127e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -421,7 +421,7 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
> atomic_dec(&inode->i_writecount);
> mutex_lock(&mapping->i_mmap_mutex);
> if (tmp->vm_flags & VM_SHARED)
> - mapping->i_mmap_writable++;
> + atomic_inc(&mapping->i_mmap_writable);
> flush_dcache_mmap_lock(mapping);
> /* insert tmp into the share list, just after mpnt */
> if (unlikely(tmp->vm_flags & VM_NONLINEAR))
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 129b847..19b6562 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -216,7 +216,7 @@ static void __remove_shared_vm_struct(struct vm_area_struct *vma,
> if (vma->vm_flags & VM_DENYWRITE)
> atomic_inc(&file_inode(file)->i_writecount);
> if (vma->vm_flags & VM_SHARED)
> - mapping->i_mmap_writable--;
> + mapping_unmap_writable(mapping);
Okay. I waste ridiculous time trying to decide whether it's better to say
mapping_unmap_writable(mapping)
or
atomic_dec(&mapping->i_mmap_writable);
here: there are consistency arguments both ways. I usually solve this
problem by not giving wrapper names to such operations, but in this case
I think the answer is just to accept whatever you decided was best.
>
> flush_dcache_mmap_lock(mapping);
> if (unlikely(vma->vm_flags & VM_NONLINEAR))
> @@ -617,7 +617,7 @@ static void __vma_link_file(struct vm_area_struct *vma)
> if (vma->vm_flags & VM_DENYWRITE)
> atomic_dec(&file_inode(file)->i_writecount);
> if (vma->vm_flags & VM_SHARED)
> - mapping->i_mmap_writable++;
> + atomic_inc(&mapping->i_mmap_writable);
>
> flush_dcache_mmap_lock(mapping);
> if (unlikely(vma->vm_flags & VM_NONLINEAR))
> @@ -1572,6 +1572,11 @@ munmap_back:
> if (error)
> goto free_vma;
> }
> + if (vm_flags & VM_SHARED) {
> + error = mapping_map_writable(file->f_mapping);
> + if (error)
> + goto allow_write_and_free_vma;
> + }
> vma->vm_file = get_file(file);
> error = file->f_op->mmap(file, vma);
> if (error)
> @@ -1611,8 +1616,12 @@ munmap_back:
>
> vma_link(mm, vma, prev, rb_link, rb_parent);
> /* Once vma denies write, undo our temporary denial count */
> - if (vm_flags & VM_DENYWRITE)
> - allow_write_access(file);
> + if (file) {
> + if (vm_flags & VM_SHARED)
> + mapping_unmap_writable(file->f_mapping);
> + if (vm_flags & VM_DENYWRITE)
> + allow_write_access(file);
> + }
> file = vma->vm_file;
Very good. A bit subtle, and for a while I was preparing to warn you
of the odd shmem_zero_setup() case (which materializes a file when
none came in), and the danger of the count going wrong on that.
But you have it handled, with the "if (file)" block immediately before
the new assignment of file, which should force anyone to think about it.
And the ordering here, with respect to file and error exits, has always
been tricky - I don't think you are making it any worse.
Oh, more subtle than I realized above: I've just gone through a last
minute "hey, it's wrong; oh, no, it's okay" waver here, remembering
how mmap /dev/zero (and some others, I suppose) gives you a new file.
That is okay: we don't even have to assume that sealing is limited
to your memfd_create(): it's safe to assume that any device which
forks you a new file, is safe to assert mapping_map_writable() upon
temporarily; and the new file given could never come already sealed.
But maybe a brief comment to reassure us in future?
> out:
> perf_event_mmap(vma);
> @@ -1641,14 +1650,17 @@ out:
> return addr;
>
> unmap_and_free_vma:
> - if (vm_flags & VM_DENYWRITE)
> - allow_write_access(file);
> vma->vm_file = NULL;
> fput(file);
>
> /* Undo any partial mapping done by a device driver. */
> unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
> charged = 0;
> + if (vm_flags & VM_SHARED)
> + mapping_unmap_writable(file->f_mapping);
> +allow_write_and_free_vma:
> + if (vm_flags & VM_DENYWRITE)
> + allow_write_access(file);
Okay. I don't enjoy that rearrangement, such that those two uses of
file come after the fput(file); but I believe there are good reasons
why it can never be the final fput(file), so I think you're okay.
> free_vma:
> kmem_cache_free(vm_area_cachep, vma);
> unacct_error:
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 2972eee..31321fa 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -39,6 +39,7 @@ static struct backing_dev_info swap_backing_dev_info = {
> struct address_space swapper_spaces[MAX_SWAPFILES] = {
> [0 ... MAX_SWAPFILES - 1] = {
> .page_tree = RADIX_TREE_INIT(GFP_ATOMIC|__GFP_NOWARN),
> + .i_mmap_writable = ATOMIC_INIT(0),
> .a_ops = &swap_aops,
> .backing_dev_info = &swap_backing_dev_info,
> }
> --
> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC v3 7/7] shm: isolate pinned pages when sealing files
From: Hugh Dickins @ 2014-07-09 8:57 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-8-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> When setting SEAL_WRITE, we must make sure nobody has a writable reference
> to the pages (via GUP or similar). We currently check references and wait
> some time for them to be dropped. This, however, might fail for several
> reasons, including:
> - the page is pinned for longer than we wait
> - while we wait, someone takes an already pinned page for read-access
>
> Therefore, this patch introduces page-isolation. When sealing a file with
> SEAL_WRITE, we copy all pages that have an elevated ref-count. The newpage
> is put in place atomically, the old page is detached and left alone. It
> will get reclaimed once the last external user dropped it.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
I've not checked it line by line, but this seems to be very good work;
and I'm glad you have posted it, where we can refer back to it in future.
However, I'm NAKing this patch, at least for now.
The reason is simple and twofold.
I absolutely do not want to be maintaining an alternative form of
page migration in mm/shmem.c. Shmem has its own peculiar problems
(mostly because of swap): adding a new dimension of very rarely
exercised complication, and dependence on the rest mm, is not wise.
And sealing just does not need this. It is clearly technically
superior to, and more satisfying than, the "wait-a-while-then-give-up"
technique which it would replace. But in practice, the wait-a-while
technique is quite good enough (and much better self-contained than this).
I've heard no requirement to support sealing of objects pinned for I/O,
and the sealer just would not have given the object out for that; the
requirement is to give the recipient of a sealed object confidence
that it cannot be susceptible to modification in that way.
I doubt there will ever be an actual need for sealing to use this
migration technique; but I can imagine us referring back to your work in
future, when/if implementing revoke on regular files. And integrating
this into mm/migrate.c's unmap_and_move() as an extra-force mode
(proceed even when the page count is raised).
I think the concerns I had, when Tony first proposed this migration copy
technique, were in fact unfounded - I was worried by the new inverse COW.
On reflection, I don't think this introduces any new risks, which are
not already present in page migration, truncation and orphaned pages.
I didn't begin to test it at all, but the only defects that stood out
in your code were in the areas of memcg and mlock. I think that if we
go down the road of duplicating pinned pages, then we do have to make
a memcg charge on the new page in addition to the old page. And if
any pages happen to be mlock'ed into an address space, then we ought
to map in the replacement pages afterwards (as page migration does,
whether mlock'ed or not).
(You were perfectly correct to use unmap_mapping_range(), rather than
try_to_unmap() as page migration does: because unmap_mapping_range()
manages the VM_NONLINEAR case. But our intention, under way, is to
scrap all VM_NONLINEAR code, and just emulate it with multiple vmas,
in which case try_to_unmap() should do.)
Hugh
> ---
> mm/shmem.c | 218 +++++++++++++++++++++++++++++--------------------------------
> 1 file changed, 105 insertions(+), 113 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index ddc3998..34b14fb 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1237,6 +1237,110 @@ unlock:
> return error;
> }
>
> +static int shmem_isolate_page(struct inode *inode, struct page *oldpage)
> +{
> + struct address_space *mapping = inode->i_mapping;
> + struct shmem_inode_info *info = SHMEM_I(inode);
> + struct page *newpage;
> + int error;
> +
> + if (oldpage->mapping != mapping)
> + return 0;
> + if (page_count(oldpage) - page_mapcount(oldpage) <= 2)
> + return 0;
> +
> + if (page_mapped(oldpage))
> + unmap_mapping_range(mapping,
> + (loff_t)oldpage->index << PAGE_CACHE_SHIFT,
> + PAGE_CACHE_SIZE, 0);
> +
> + VM_BUG_ON_PAGE(PageWriteback(oldpage), oldpage);
> + VM_BUG_ON_PAGE(page_has_private(oldpage), oldpage);
> +
> + newpage = shmem_alloc_page(mapping_gfp_mask(mapping), info,
> + oldpage->index);
> + if (!newpage)
> + return -ENOMEM;
> +
> + __set_page_locked(newpage);
> + copy_highpage(newpage, oldpage);
> + flush_dcache_page(newpage);
> +
> + page_cache_get(newpage);
> + SetPageUptodate(newpage);
> + SetPageSwapBacked(newpage);
> + newpage->mapping = mapping;
> + newpage->index = oldpage->index;
> +
> + cancel_dirty_page(oldpage, PAGE_CACHE_SIZE);
> +
> + spin_lock_irq(&mapping->tree_lock);
> + error = shmem_radix_tree_replace(mapping, oldpage->index,
> + oldpage, newpage);
> + if (!error) {
> + __inc_zone_page_state(newpage, NR_FILE_PAGES);
> + __dec_zone_page_state(oldpage, NR_FILE_PAGES);
> + }
> + spin_unlock_irq(&mapping->tree_lock);
> +
> + if (error) {
> + newpage->mapping = NULL;
> + unlock_page(newpage);
> + page_cache_release(newpage);
> + page_cache_release(newpage);
> + return error;
> + }
> +
> + mem_cgroup_replace_page_cache(oldpage, newpage);
> + lru_cache_add_anon(newpage);
> +
> + oldpage->mapping = NULL;
> + page_cache_release(oldpage);
> + unlock_page(newpage);
> + page_cache_release(newpage);
> +
> + return 1;
> +}
> +
> +static int shmem_isolate_pins(struct inode *inode)
> +{
> + struct address_space *mapping = inode->i_mapping;
> + struct pagevec pvec;
> + pgoff_t indices[PAGEVEC_SIZE];
> + pgoff_t index;
> + int i, ret, error;
> +
> + pagevec_init(&pvec, 0);
> + index = 0;
> + error = 0;
> + while ((pvec.nr = find_get_entries(mapping, index, PAGEVEC_SIZE,
> + pvec.pages, indices))) {
> + for (i = 0; i < pagevec_count(&pvec); i++) {
> + struct page *page = pvec.pages[i];
> +
> + index = indices[i];
> + if (radix_tree_exceptional_entry(page))
> + continue;
> + if (page->mapping != mapping)
> + continue;
> + if (page_count(page) - page_mapcount(page) <= 2)
> + continue;
> +
> + lock_page(page);
> + ret = shmem_isolate_page(inode, page);
> + if (ret < 0)
> + error = ret;
> + unlock_page(page);
> + }
> + pagevec_remove_exceptionals(&pvec);
> + pagevec_release(&pvec);
> + cond_resched();
> + index++;
> + }
> +
> + return error;
> +}
> +
> static int shmem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> {
> struct inode *inode = file_inode(vma->vm_file);
> @@ -1734,118 +1838,6 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> return offset;
> }
>
> -/*
> - * We need a tag: a new tag would expand every radix_tree_node by 8 bytes,
> - * so reuse a tag which we firmly believe is never set or cleared on shmem.
> - */
> -#define SHMEM_TAG_PINNED PAGECACHE_TAG_TOWRITE
> -#define LAST_SCAN 4 /* about 150ms max */
> -
> -static void shmem_tag_pins(struct address_space *mapping)
> -{
> - struct radix_tree_iter iter;
> - void **slot;
> - pgoff_t start;
> - struct page *page;
> -
> - start = 0;
> - rcu_read_lock();
> -
> -restart:
> - radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
> - page = radix_tree_deref_slot(slot);
> - if (!page || radix_tree_exception(page)) {
> - if (radix_tree_deref_retry(page))
> - goto restart;
> - } else if (page_count(page) - page_mapcount(page) > 1) {
> - spin_lock_irq(&mapping->tree_lock);
> - radix_tree_tag_set(&mapping->page_tree, iter.index,
> - SHMEM_TAG_PINNED);
> - spin_unlock_irq(&mapping->tree_lock);
> - }
> -
> - if (need_resched()) {
> - cond_resched_rcu();
> - start = iter.index + 1;
> - goto restart;
> - }
> - }
> - rcu_read_unlock();
> -}
> -
> -/*
> - * Setting SEAL_WRITE requires us to verify there's no pending writer. However,
> - * via get_user_pages(), drivers might have some pending I/O without any active
> - * user-space mappings (eg., direct-IO, AIO). Therefore, we look at all pages
> - * and see whether it has an elevated ref-count. If so, we tag them and wait for
> - * them to be dropped.
> - * The caller must guarantee that no new user will acquire writable references
> - * to those pages to avoid races.
> - */
> -static int shmem_wait_for_pins(struct address_space *mapping)
> -{
> - struct radix_tree_iter iter;
> - void **slot;
> - pgoff_t start;
> - struct page *page;
> - int error, scan;
> -
> - shmem_tag_pins(mapping);
> -
> - error = 0;
> - for (scan = 0; scan <= LAST_SCAN; scan++) {
> - if (!radix_tree_tagged(&mapping->page_tree, SHMEM_TAG_PINNED))
> - break;
> -
> - if (!scan)
> - lru_add_drain_all();
> - else if (schedule_timeout_killable((HZ << scan) / 200))
> - scan = LAST_SCAN;
> -
> - start = 0;
> - rcu_read_lock();
> -restart:
> - radix_tree_for_each_tagged(slot, &mapping->page_tree, &iter,
> - start, SHMEM_TAG_PINNED) {
> -
> - page = radix_tree_deref_slot(slot);
> - if (radix_tree_exception(page)) {
> - if (radix_tree_deref_retry(page))
> - goto restart;
> -
> - page = NULL;
> - }
> -
> - if (page &&
> - page_count(page) - page_mapcount(page) != 1) {
> - if (scan < LAST_SCAN)
> - goto continue_resched;
> -
> - /*
> - * On the last scan, we clean up all those tags
> - * we inserted; but make a note that we still
> - * found pages pinned.
> - */
> - error = -EBUSY;
> - }
> -
> - spin_lock_irq(&mapping->tree_lock);
> - radix_tree_tag_clear(&mapping->page_tree,
> - iter.index, SHMEM_TAG_PINNED);
> - spin_unlock_irq(&mapping->tree_lock);
> -continue_resched:
> - if (need_resched()) {
> - cond_resched_rcu();
> - start = iter.index + 1;
> - goto restart;
> - }
> - }
> - rcu_read_unlock();
> - }
> -
> - return error;
> -}
> -
> #define F_ALL_SEALS (F_SEAL_SEAL | \
> F_SEAL_SHRINK | \
> F_SEAL_GROW | \
> @@ -1907,7 +1899,7 @@ int shmem_add_seals(struct file *file, unsigned int seals)
> if (error)
> goto unlock;
>
> - error = shmem_wait_for_pins(file->f_mapping);
> + error = shmem_isolate_pins(inode);
> if (error) {
> mapping_allow_writable(file->f_mapping);
> goto unlock;
> --
> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v12 7/8] arm64: add pmd_[dirty|mkclean] for THP
From: Catalin Marinas @ 2014-07-09 11:14 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-kernel@vger.kernel.org, linux-mm@kvack.org,
Michael Kerrisk, Linux API, Hugh Dickins, Johannes Weiner,
Rik van Riel, KOSAKI Motohiro, Mel Gorman, Jason Evans,
Zhang Yanfei, Kirill A. Shutemov, Will Deacon, Steve Capper,
Russell King, linux-arm-kernel@lists.infradead.org
In-Reply-To: <1404886949-17695-8-git-send-email-minchan@kernel.org>
On Wed, Jul 09, 2014 at 07:22:28AM +0100, Minchan Kim wrote:
> MADV_FREE needs pmd_dirty and pmd_mkclean for detecting recent
> overwrite of the contents since MADV_FREE syscall is called for
> THP page.
>
> This patch adds pmd_dirty and pmd_mkclean for THP page MADV_FREE
> support.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Steve Capper <steve.capper@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [net-next PATCH v2 3/3] drivers: net: cpsw: Add support for multicast/broadcast rate limit
From: Govindarajulu Varadarajan @ 2014-07-09 13:39 UTC (permalink / raw)
To: Mugunthan V N
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1404890050-4020-4-git-send-email-mugunthanvnm-l0cyMroinI0@public.gmane.org>
On Wed, 9 Jul 2014, Mugunthan V N wrote:
> Add support for multicast/broadcast rate limit feature via ethtool coalesce.
>
> Signed-off-by: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
> ---
> drivers/net/ethernet/ti/cpsw.c | 80 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index a6117e6..2a984e6 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -403,6 +403,8 @@ struct cpsw_priv {
> bool irq_enabled;
> struct cpts *cpts;
> u32 emac_port;
> + u32 rx_max_mcast;
> + u32 rx_max_bcast;
> };
>
> struct cpsw_stats {
> @@ -871,11 +873,13 @@ static int cpsw_get_coalesce(struct net_device *ndev,
> struct cpsw_priv *priv = netdev_priv(ndev);
>
> coal->rx_coalesce_usecs = priv->coal_intvl;
> + coal->rx_max_mcast = priv->rx_max_mcast;
> + coal->rx_max_bcast = priv->rx_max_bcast;
> return 0;
> }
>
> -static int cpsw_set_coalesce(struct net_device *ndev,
> - struct ethtool_coalesce *coal)
> +static int cpsw_set_coalesce_usecs(struct net_device *ndev,
> + struct ethtool_coalesce *coal)
> {
> struct cpsw_priv *priv = netdev_priv(ndev);
> u32 int_ctrl;
> @@ -933,6 +937,74 @@ static int cpsw_set_coalesce(struct net_device *ndev,
> return 0;
> }
>
> +static int cpsw_set_coalesce_mcast(struct net_device *ndev,
> + struct ethtool_coalesce *coal)
> +{
> + struct cpsw_priv *priv = netdev_priv(ndev);
> + int port;
> +
> + priv->rx_max_mcast = coal->rx_max_mcast;
> +
> + if (priv->data.dual_emac)
> + port = priv->emac_port;
> + else
> + port = priv->data.active_slave;
> +
> + cpsw_ale_control_set(priv->ale, port, ALE_PORT_MCAST_LIMIT,
> + coal->rx_max_mcast);
> +
> + dev_dbg(priv->dev, "rx_max_mcast set to %d\n", priv->rx_max_mcast);
> + return 0;
> +}
> +
> +static int cpsw_set_coalesce_bcast(struct net_device *ndev,
> + struct ethtool_coalesce *coal)
> +{
> + struct cpsw_priv *priv = netdev_priv(ndev);
> + int port;
> +
> + priv->rx_max_bcast = coal->rx_max_bcast;
> +
> + if (priv->data.dual_emac)
> + port = priv->emac_port + 1;
> + else
> + port = priv->data.active_slave + 1;
> +
> + cpsw_ale_control_set(priv->ale, port, ALE_PORT_BCAST_LIMIT,
> + coal->rx_max_bcast);
Both cpsw_set_coalesce_bcast & cpsw_set_coalesce_mcast always return 0.
May be you can return result of cpsw_ale_control_set()?
> +
> + dev_dbg(priv->dev, "rx_max_mcast set to %d\n", priv->rx_max_bcast);
> + return 0;
> +}
> +
> +static int cpsw_set_coalesce(struct net_device *ndev,
> + struct ethtool_coalesce *coal)
> +{
> + int ret = -EINVAL;
> +
> + if (coal->rx_coalesce_usecs) {
> + ret = cpsw_set_coalesce_usecs(ndev, coal);
> + if (ret) {
> + dev_err(&ndev->dev, "set rx-usecs failed\n");
> + return ret;
> + }
> + }
> +
> + ret = cpsw_set_coalesce_mcast(ndev, coal);
> + if (ret) {
> + dev_err(&ndev->dev, "set coalesce rx-max-mcast failed\n");
> + return ret;
> + }
So that ret check here will be meaningful.
thanks
> +
> + ret = cpsw_set_coalesce_bcast(ndev, coal);
> + if (ret) {
> + dev_err(&ndev->dev, "set coalesce rx-max-bcast failed\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> static int cpsw_get_sset_count(struct net_device *ndev, int sset)
> {
> switch (sset) {
> @@ -1227,6 +1299,10 @@ static int cpsw_ndo_open(struct net_device *ndev)
> /* enable statistics collection only on all ports */
> __raw_writel(0x7, &priv->regs->stat_port_en);
>
> + /* Enable rate limit feature in the switch for rx only */
> + cpsw_ale_control_set(priv->ale, 0, ALE_RATE_LIMIT, 1);
> + cpsw_ale_control_set(priv->ale, 0, ALE_RATE_LIMIT_TX, 0);
> +
> if (WARN_ON(!priv->data.rx_descs))
> priv->data.rx_descs = 128;
>
> --
> 2.0.0.390.gcb682f8
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v9 11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Oleg Nesterov @ 2014-07-09 18:05 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403911380-27787-12-git-send-email-keescook@chromium.org>
First of all, sorry for delay ;)
So far I quickly glanced at this series and everything look fine, but
I am confused by the signal_group_exit() check,
On 06/27, Kees Cook wrote:
>
> To make sure that de_thread() is actually able
> to kill other threads during an exec, any sighand holders need to check
> if they've been scheduled to be killed, and to give up on their work.
Probably this connects to that check below? I can't understand this...
> + /*
> + * Make sure we cannot change seccomp or nnp state via TSYNC
> + * while another thread is in the middle of calling exec.
> + */
> + if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
> + mutex_lock_killable(¤t->signal->cred_guard_mutex))
> + goto out_free;
-EINVAL looks a bit confusing in this case, but this is cosemtic because
userspace won't see this error-code anyway.
> spin_lock_irq(¤t->sighand->siglock);
> + if (unlikely(signal_group_exit(current->signal))) {
> + /* If thread is dying, return to process the signal. */
OK, this doesn't hurt, but why?
You could check __fatal_signal_pending() with the same effect. And since
we hold this mutex, exec (de_thread) can be the source of that SIGKILL.
We take this mutex specially to avoid the race with exec.
So why do we need to abort if we race with kill() or exit_grouo() ?
Oleg.
^ permalink raw reply
* Re: [PATCH v9 09/11] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-07-09 18:42 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403911380-27787-10-git-send-email-keescook@chromium.org>
On 06/27, Kees Cook wrote:
>
> static u32 seccomp_run_filters(int syscall)
> {
> - struct seccomp_filter *f;
> + struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);
I am not sure...
This is fine if this ->filter is the 1st (and only) one, in this case
we can rely on rmb() in the caller.
But the new filter can be installed at any moment. Say, right after that
rmb() although this doesn't matter. Either we need smp_read_barrier_depends()
after that, or smp_load_acquire() like the previous version did?
Oleg.
^ permalink raw reply
* Re: [PATCH v9 09/11] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-07-09 18:55 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
linux-mips, linux-arch, linux-security-module
In-Reply-To: <20140709184215.GA4866@redhat.com>
On 07/09, Oleg Nesterov wrote:
>
> On 06/27, Kees Cook wrote:
> >
> > static u32 seccomp_run_filters(int syscall)
> > {
> > - struct seccomp_filter *f;
> > + struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);
>
> I am not sure...
>
> This is fine if this ->filter is the 1st (and only) one, in this case
> we can rely on rmb() in the caller.
>
> But the new filter can be installed at any moment. Say, right after that
> rmb() although this doesn't matter. Either we need smp_read_barrier_depends()
> after that, or smp_load_acquire() like the previous version did?
Wait... and it seems that seccomp_sync_threads() needs smp_store_release()
when it sets thread->filter = current->filter by the same reason?
OTOH. smp_store_release() in seccomp_attach_filter() can die, "current"
doesn't need a barrier to serialize with itself.
Oleg.
^ permalink raw reply
* Re: [PATCH v9 09/11] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-07-09 18:59 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, linux-api, x86, linux-arm-kernel,
linux-mips, linux-arch, linux-security-module
In-Reply-To: <1403911380-27787-10-git-send-email-keescook@chromium.org>
On 06/27, Kees Cook wrote:
>
> void put_seccomp_filter(struct task_struct *tsk)
> {
> - struct seccomp_filter *orig = tsk->seccomp.filter;
> + struct seccomp_filter *orig = smp_load_acquire(&tsk->seccomp.filter);
> /* Clean up single-reference branches iteratively. */
> while (orig && atomic_dec_and_test(&orig->usage)) {
And this smp_load_acquire() looks unnecessary. atomic_dec_and_test()
is a barrier.
Oleg.
^ permalink raw reply
* Re: [net-next PATCH v2 0/3] Broadcast/Multicast rate limit via Ethtool Coalesce
From: David Miller @ 2014-07-09 23:44 UTC (permalink / raw)
To: mugunthanvnm-l0cyMroinI0
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1404890050-4020-1-git-send-email-mugunthanvnm-l0cyMroinI0@public.gmane.org>
From: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
Date: Wed, 9 Jul 2014 12:44:07 +0530
> A system/cpu can be loaded by a hacker with flooding of broadcast or
> multicast packets, to prevent this some Ethernet controllers like CPSW
> provide a mechanism to limit the broadcast/multicast packet rate via
> hardware limiters. This patch series enables this feature via
> Ethtool Coalesce.
This is pretty bogus if you ask me.
What is the difference from accepting a high rate of unicast packets?
I say it is no different.
Therefore, this feature makes no sense to me at all.
^ permalink raw reply
* Re: [PATCH v2 2/2] man2: Document constant for only allowing absolute paths
From: Steven Stewart-Gallus @ 2014-07-09 23:53 UTC (permalink / raw)
To: Steven Stewart-Gallus
Cc: Christoph Hellwig, Jeff Layton, J. Bruce Fields,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andy Lutomirski
In-Reply-To: <fb4eb72f7cd0.53bdd56f-BTv7Ps/Sm75C8prJL3GQQw@public.gmane.org>
Signed-off-by: Steven Stewart-Gallus <sstewartgallus00-QKvm5KDIoDa7M0a00MdBSQ@public.gmane.org>
---
man2/access.2 | 14 +++++++++++++-
man2/chmod.2 | 14 +++++++++++++-
man2/chown.2 | 14 +++++++++++++-
man2/fanotify_mark.2 | 15 ++++++++++++++-
man2/futimesat.2 | 14 +++++++++++++-
man2/link.2 | 11 ++++++++++-
man2/mkdir.2 | 14 +++++++++++++-
man2/mknod.2 | 14 +++++++++++++-
man2/open.2 | 14 +++++++++++++-
man2/open_by_handle_at.2 | 8 ++++++++
man2/readlink.2 | 14 +++++++++++++-
man2/rename.2 | 11 ++++++++++-
man2/stat.2 | 14 +++++++++++++-
man2/symlink.2 | 14 +++++++++++++-
man2/unlink.2 | 14 +++++++++++++-
man2/utimensat.2 | 8 ++++++++
man3/readdir.3 | 18 ++++++++++++++----
17 files changed, 207 insertions(+), 18 deletions(-)
diff --git a/man2/access.2 b/man2/access.2
index cdf2f96..f2a030b 100644
--- a/man2/access.2
+++ b/man2/access.2
@@ -141,6 +141,14 @@ directory of the calling process (like
.BR access ()).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -244,7 +252,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B EINVAL
Invalid flag specified in
diff --git a/man2/chmod.2 b/man2/chmod.2
index b46c0fa..1cde0f1 100644
--- a/man2/chmod.2
+++ b/man2/chmod.2
@@ -209,6 +209,14 @@ directory of the calling process (like
.BR chmod ()).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -306,7 +314,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B EINVAL
Invalid flag specified in
diff --git a/man2/chown.2 b/man2/chown.2
index 8f6194e..aafadc5 100644
--- a/man2/chown.2
+++ b/man2/chown.2
@@ -172,6 +172,14 @@ directory of the calling process (like
.BR chown ()).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -295,7 +303,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B EINVAL
Invalid flag specified in
diff --git a/man2/fanotify_mark.2 b/man2/fanotify_mark.2
index d3c7e7d..13bf51d 100644
--- a/man2/fanotify_mark.2
+++ b/man2/fanotify_mark.2
@@ -214,6 +214,14 @@ is absolute, it defines the filesystem object to be marked, and
is ignored.
.IP *
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+.IP *
+If
.I pathname
is relative, and
.I dirfd
@@ -244,7 +252,12 @@ is set to indicate the error.
.TP
.B EBADF
An invalid file descriptor was passed in
-.IR fanotify_fd .
+.IR fanotify_fd
+or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B EINVAL
An invalid value was passed in
diff --git a/man2/futimesat.2 b/man2/futimesat.2
index e6e0174..2b9298d 100644
--- a/man2/futimesat.2
+++ b/man2/futimesat.2
@@ -77,6 +77,14 @@ directory of the calling process (like
.BR utimes (2)).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -98,7 +106,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B ENOTDIR
.I pathname
diff --git a/man2/link.2 b/man2/link.2
index 0725cd2..d340235 100644
--- a/man2/link.2
+++ b/man2/link.2
@@ -107,6 +107,14 @@ directory of the calling process (like
.BR link ()).
If
+.I olddirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I oldpath
+must be absolute.
+
+If
.I oldpath
is absolute, then
.I olddirfd
@@ -278,7 +286,8 @@ The following additional errors can occur for
.I olddirfd
or
.I newdirfd
-is not a valid file descriptor.
+is not a valid file descriptor or one of them is AT_FDABSOLUTE and the
+associated path is relative.
.TP
.B EINVAL
An invalid flag value was specified in
diff --git a/man2/mkdir.2 b/man2/mkdir.2
index 71f794f..4c18fcb 100644
--- a/man2/mkdir.2
+++ b/man2/mkdir.2
@@ -103,6 +103,14 @@ directory of the calling process (like
.BR mkdir ()).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -190,7 +198,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B ENOTDIR
.I pathname
diff --git a/man2/mknod.2 b/man2/mknod.2
index e93c345..4b8b620 100644
--- a/man2/mknod.2
+++ b/man2/mknod.2
@@ -137,6 +137,14 @@ directory of the calling process (like
.BR mknod (2)).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -230,7 +238,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B ENOTDIR
.I pathname
diff --git a/man2/open.2 b/man2/open.2
index df10a22..c65d705 100644
--- a/man2/open.2
+++ b/man2/open.2
@@ -767,6 +767,14 @@ directory of the calling process (like
.BR open ()).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -998,7 +1006,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B ENOTDIR
.I pathname
diff --git a/man2/open_by_handle_at.2 b/man2/open_by_handle_at.2
index 7badac7..70fafb3 100644
--- a/man2/open_by_handle_at.2
+++ b/man2/open_by_handle_at.2
@@ -181,6 +181,14 @@ or
.BR AT_FDCWD ,
meaning the current working directory,
and a handle is returned for the file to which it refers.
+.IP *
+If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
.PP
The
.I mount_id
diff --git a/man2/readlink.2 b/man2/readlink.2
index 19102e1..e5a0e13 100644
--- a/man2/readlink.2
+++ b/man2/readlink.2
@@ -127,6 +127,14 @@ directory of the calling process (like
.BR readlink ()).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -202,7 +210,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B ENOTDIR
.I pathname
diff --git a/man2/rename.2 b/man2/rename.2
index a258972..c857886 100644
--- a/man2/rename.2
+++ b/man2/rename.2
@@ -151,6 +151,14 @@ directory of the calling process (like
.BR rename ()).
If
+.I olddirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I oldpath
+must be absolute.
+
+If
.I oldpath
is absolute, then
.I olddirfd
@@ -351,7 +359,8 @@ and
.I olddirfd
or
.I newdirfd
-is not a valid file descriptor.
+is not a valid file descriptor or one of them is AT_FDABSOLUTE and the
+associated path is relative.
.TP
.B ENOTDIR
.I oldpath
diff --git a/man2/stat.2 b/man2/stat.2
index b70797d..c210b0d 100644
--- a/man2/stat.2
+++ b/man2/stat.2
@@ -452,6 +452,14 @@ directory of the calling process (like
.BR stat ()).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
@@ -581,7 +589,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B EINVAL
Invalid flag specified in
diff --git a/man2/symlink.2 b/man2/symlink.2
index 8bd067f..b53ca2d 100644
--- a/man2/symlink.2
+++ b/man2/symlink.2
@@ -133,6 +133,14 @@ directory of the calling process (like
.BR symlink ()).
If
+.I newdirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I linkpath
+must be absolute.
+
+If
.I linkpath
is absolute, then
.I newdirfd
@@ -208,7 +216,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I newdirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I newdirfd
+is AT_FDABSOLUTE and
+.I linkpath
+is relative.
.TP
.B ENOENT
.I linkpath
diff --git a/man2/unlink.2 b/man2/unlink.2
index ccc05bf..b6c1591 100644
--- a/man2/unlink.2
+++ b/man2/unlink.2
@@ -119,6 +119,14 @@ directory of the calling process (like
and
.BR rmdir (2)).
+If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
If the pathname given in
.I pathname
is absolute, then
@@ -248,7 +256,11 @@ The following additional errors can occur for
.TP
.B EBADF
.I dirfd
-is not a valid file descriptor.
+is not a valid file descriptor or
+.I dirfd
+is AT_FDABSOLUTE and
+.I pathname
+is relative.
.TP
.B EINVAL
An invalid flag value was specified in
diff --git a/man2/utimensat.2 b/man2/utimensat.2
index 99b985c..c22e1f3 100644
--- a/man2/utimensat.2
+++ b/man2/utimensat.2
@@ -206,6 +206,14 @@ directory of the calling process (like
.BR utimes (2)).
If
+.I dirfd
+is the special value
+.BR AT_FDABSOLUTE ,
+then
+.I pathname
+must be absolute.
+
+If
.I pathname
is absolute, then
.I dirfd
diff --git a/man3/readdir.3 b/man3/readdir.3
index 898ab31..7eda4a7 100644
--- a/man3/readdir.3
+++ b/man3/readdir.3
@@ -250,10 +250,10 @@ as follows:
.in +4n
.nf
-name_max = pathconf(dirpath, _PC_NAME_MAX);
+name_max = fpathconf(fddir, _PC_NAME_MAX);
if (name_max == \-1) /* Limit not defined, or error */
- name_max = 255; /* Take a guess */
-len = offsetof(struct dirent, d_name) + name_max + 1;
+ return ENOSYS; /* Do not take a guess, that is incorrect */
+len = offsetof(struct dirent, d_name) + name_max + 1u;
entryp = malloc(len);
.fi
@@ -261,7 +261,17 @@ entryp = malloc(len);
(POSIX.1 requires that
.I d_name
is the last field in a
-.IR "struct dirent" .)
+.IR "struct dirent" ".)"
+Note that
+.I fpathconf
+is used instead of
+.I pathconf
+to avoid a time of check to time of use security hole. As well,
+.I dirfd
+is not portable to all systems so
+.I readdir_r
+therefore can not be used in a fully portable way. This problem is
+currently under review by the Austin Common Standards Revision Group.
.SH SEE ALSO
.BR getdents (2),
.BR read (2),
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v9 11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-07-10 9:17 UTC (permalink / raw)
To: Oleg Nesterov
Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <20140709180520.GA2560@redhat.com>
On Wed, Jul 9, 2014 at 11:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> First of all, sorry for delay ;)
>
> So far I quickly glanced at this series and everything look fine, but
> I am confused by the signal_group_exit() check,
>
> On 06/27, Kees Cook wrote:
>>
>> To make sure that de_thread() is actually able
>> to kill other threads during an exec, any sighand holders need to check
>> if they've been scheduled to be killed, and to give up on their work.
>
> Probably this connects to that check below? I can't understand this...
>
>> + /*
>> + * Make sure we cannot change seccomp or nnp state via TSYNC
>> + * while another thread is in the middle of calling exec.
>> + */
>> + if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
>> + mutex_lock_killable(¤t->signal->cred_guard_mutex))
>> + goto out_free;
>
> -EINVAL looks a bit confusing in this case, but this is cosemtic because
> userspace won't see this error-code anyway.
Happy to use whatever since, as you say, it's cosmetic. Perhaps -EAGAIN?
>
>> spin_lock_irq(¤t->sighand->siglock);
>> + if (unlikely(signal_group_exit(current->signal))) {
>> + /* If thread is dying, return to process the signal. */
>
> OK, this doesn't hurt, but why?
>
> You could check __fatal_signal_pending() with the same effect. And since
> we hold this mutex, exec (de_thread) can be the source of that SIGKILL.
> We take this mutex specially to avoid the race with exec.
>
> So why do we need to abort if we race with kill() or exit_grouo() ?
In my initial code inspection that we could block waiting for the
cred_guard mutex, with exec holding it, exec would schedule death in
de_thread, and then once it released, the tsync thread would try to
keep running.
However, in looking at this again, now I'm concerned this produces a
dead-lock in de_thread, since it waits for all threads to actually
die, but tsync will be waiting with the killable mutex.
So I think I got too defensive when I read the top of de_thread where
it checks for pending signals itself.
It seems like I can just safely remove the singal_group_exit checks?
The other paths (non-tsync seccomp_set_mode_filter, and
seccomp_set_mode_strict) would just run until it finished the syscall,
and then died. I can't decide which feels cleaner: just letting stuff
clean up naturally on death or to short-circuit after taking
sighand->siglock.
What do you think?
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v9 09/11] seccomp: introduce writer locking
From: Kees Cook @ 2014-07-10 9:25 UTC (permalink / raw)
To: Oleg Nesterov
Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <20140709185549.GB4866@redhat.com>
On Wed, Jul 9, 2014 at 11:55 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/09, Oleg Nesterov wrote:
>>
>> On 06/27, Kees Cook wrote:
>> >
>> > static u32 seccomp_run_filters(int syscall)
>> > {
>> > - struct seccomp_filter *f;
>> > + struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);
>>
>> I am not sure...
>>
>> This is fine if this ->filter is the 1st (and only) one, in this case
>> we can rely on rmb() in the caller.
>>
>> But the new filter can be installed at any moment. Say, right after that
>> rmb() although this doesn't matter. Either we need smp_read_barrier_depends()
>> after that, or smp_load_acquire() like the previous version did?
>
> Wait... and it seems that seccomp_sync_threads() needs smp_store_release()
> when it sets thread->filter = current->filter by the same reason?
>
> OTOH. smp_store_release() in seccomp_attach_filter() can die, "current"
> doesn't need a barrier to serialize with itself.
I have lost track of what you're suggesting to change. :)
Since rmb() happens before run_filters, isn't the ACCESS_ONCE
sufficient? We only care that TIF_SECCOMP, mode, and some filter is
valid. In a tsync thread race, it's okay to use not use the deepest
filter node in the list, it just needs A filter.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v9 11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Oleg Nesterov @ 2014-07-10 15:08 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5jLUqz-T1tRBCpPLkzWijyAF-Vjw_7PnQ1EvUh4urwyaUg@mail.gmail.com>
On 07/10, Kees Cook wrote:
>
> On Wed, Jul 9, 2014 at 11:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> >> + /*
> >> + * Make sure we cannot change seccomp or nnp state via TSYNC
> >> + * while another thread is in the middle of calling exec.
> >> + */
> >> + if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
> >> + mutex_lock_killable(¤t->signal->cred_guard_mutex))
> >> + goto out_free;
> >
> > -EINVAL looks a bit confusing in this case, but this is cosemtic because
> > userspace won't see this error-code anyway.
>
> Happy to use whatever since, as you say, it's cosmetic. Perhaps -EAGAIN?
Or -EINTR. I do not really mind, I only mentioned this because I had another
nit.
> >> spin_lock_irq(¤t->sighand->siglock);
> >> + if (unlikely(signal_group_exit(current->signal))) {
> >> + /* If thread is dying, return to process the signal. */
> >
> > OK, this doesn't hurt, but why?
> >
> > You could check __fatal_signal_pending() with the same effect. And since
> > we hold this mutex, exec (de_thread) can be the source of that SIGKILL.
> > We take this mutex specially to avoid the race with exec.
> >
> > So why do we need to abort if we race with kill() or exit_grouo() ?
>
> In my initial code inspection that we could block waiting for the
> cred_guard mutex, with exec holding it, exec would schedule death in
> de_thread, and then once it released, the tsync thread would try to
> keep running.
>
> However, in looking at this again, now I'm concerned this produces a
> dead-lock in de_thread, since it waits for all threads to actually
> die, but tsync will be waiting with the killable mutex.
That is why you should always use _killable (or _interruptible) if you
want to take ->cred_guard_mutex.
If this thread races with de_thread() which holds this mutex, it will
be killed and mutex_lock_killable() will fail.
(to clarify; this deadlock is not "fatal", de_thread() can be killed too,
but this doesn't really matter).
> So I think I got too defensive when I read the top of de_thread where
> it checks for pending signals itself.
>
> It seems like I can just safely remove the singal_group_exit checks?
> The other paths (non-tsync seccomp_set_mode_filter, and
> seccomp_set_mode_strict)
Yes, I missed another signal_group_exit() in seccomp_set_mode_strict().
It looks equally unneeded.
> I can't decide which feels cleaner: just letting stuff
> clean up naturally on death or to short-circuit after taking
> sighand->siglock.
I'd prefer to simply remove the singal_group_exit checks.
I won't argue if you prefer to keep them, but then please add a comment
to explain that this is not needed for correctness.
Because otherwise the code looks confusing, as if there is a subtle reason
why we must not do this if killed.
Oleg.
^ permalink raw reply
* Re: [PATCH v9 09/11] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-07-10 15:24 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API, x86@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <CAGXu5jL6q1d16uA1Yu+QO4eV7zWwcWEWgkZrwmsfymbMvEr6+Q@mail.gmail.com>
On 07/10, Kees Cook wrote:
>
> On Wed, Jul 9, 2014 at 11:55 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 07/09, Oleg Nesterov wrote:
> >>
> >> On 06/27, Kees Cook wrote:
> >> >
> >> > static u32 seccomp_run_filters(int syscall)
> >> > {
> >> > - struct seccomp_filter *f;
> >> > + struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);
> >>
> >> I am not sure...
> >>
> >> This is fine if this ->filter is the 1st (and only) one, in this case
> >> we can rely on rmb() in the caller.
> >>
> >> But the new filter can be installed at any moment. Say, right after that
> >> rmb() although this doesn't matter. Either we need smp_read_barrier_depends()
> >> after that, or smp_load_acquire() like the previous version did?
> >
> > Wait... and it seems that seccomp_sync_threads() needs smp_store_release()
> > when it sets thread->filter = current->filter by the same reason?
> >
> > OTOH. smp_store_release() in seccomp_attach_filter() can die, "current"
> > doesn't need a barrier to serialize with itself.
>
> I have lost track of what you're suggesting to change. :)
Perhaps I am just trying to confuse you and myself ;)
But,
> Since rmb() happens before run_filters, isn't the ACCESS_ONCE
> sufficient?
Yes. But see above. ACCESS_ONCE is sufficient if we read the first filter
installed by another thread, in this case rmb() pairs with mb_before_atomic()
before set_bit(TIF_SECCOMP).
IOW, if this threads sees TIF_SECCOMP, it should also see all modifications
which were done before set_bit, including the data in ->filter points to.
> We only care that TIF_SECCOMP, mode, and some filter is
> valid. In a tsync thread race, it's okay to use not use the deepest
> filter node in the list,
Yes, it is fine if we miss yet another filter which was just installed by
another thread.
But, unless I missed something, the problem is that we can get this new
filter.
Just to simplify. Suppose TIF_SECCOMP was set a long ago. This thread
has a single filter F1 and it enters seccomp_run_filters().
Right before it does ACCESS_ONCE() to read the pointer, another thread
does seccomp_sync_threads() and sets .filter = F2.
If ACCESS_ONCE() returns F1 - everything is fine. But it can see the new
pointer F2, and in this case we need a barrier to ensure that, say,
LOAD(F2->prog) will see all the preceding changes in this memory.
Oleg.
^ permalink raw reply
* Re: [PATCH v9 11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-07-10 16:03 UTC (permalink / raw)
To: Oleg Nesterov
Cc: LKML, Andy Lutomirski, Michael Kerrisk (man-pages),
Alexei Starovoitov, Andrew Morton, Daniel Borkmann, Will Drewry,
Julien Tinnes, David Drysdale, Linux API,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mips-6z/3iImG2C8G8FEW9MqTrA, linux-arch,
linux-security-module
In-Reply-To: <20140710150832.GA20861-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Thu, Jul 10, 2014 at 8:08 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 07/10, Kees Cook wrote:
>>
>> On Wed, Jul 9, 2014 at 11:05 AM, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> >
>> >> + /*
>> >> + * Make sure we cannot change seccomp or nnp state via TSYNC
>> >> + * while another thread is in the middle of calling exec.
>> >> + */
>> >> + if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
>> >> + mutex_lock_killable(¤t->signal->cred_guard_mutex))
>> >> + goto out_free;
>> >
>> > -EINVAL looks a bit confusing in this case, but this is cosemtic because
>> > userspace won't see this error-code anyway.
>>
>> Happy to use whatever since, as you say, it's cosmetic. Perhaps -EAGAIN?
>
> Or -EINTR. I do not really mind, I only mentioned this because I had another
> nit.
>
>> >> spin_lock_irq(¤t->sighand->siglock);
>> >> + if (unlikely(signal_group_exit(current->signal))) {
>> >> + /* If thread is dying, return to process the signal. */
>> >
>> > OK, this doesn't hurt, but why?
>> >
>> > You could check __fatal_signal_pending() with the same effect. And since
>> > we hold this mutex, exec (de_thread) can be the source of that SIGKILL.
>> > We take this mutex specially to avoid the race with exec.
>> >
>> > So why do we need to abort if we race with kill() or exit_grouo() ?
>>
>> In my initial code inspection that we could block waiting for the
>> cred_guard mutex, with exec holding it, exec would schedule death in
>> de_thread, and then once it released, the tsync thread would try to
>> keep running.
>>
>> However, in looking at this again, now I'm concerned this produces a
>> dead-lock in de_thread, since it waits for all threads to actually
>> die, but tsync will be waiting with the killable mutex.
>
> That is why you should always use _killable (or _interruptible) if you
> want to take ->cred_guard_mutex.
>
> If this thread races with de_thread() which holds this mutex, it will
> be killed and mutex_lock_killable() will fail.
>
> (to clarify; this deadlock is not "fatal", de_thread() can be killed too,
> but this doesn't really matter).
>
>> So I think I got too defensive when I read the top of de_thread where
>> it checks for pending signals itself.
>>
>> It seems like I can just safely remove the singal_group_exit checks?
>> The other paths (non-tsync seccomp_set_mode_filter, and
>> seccomp_set_mode_strict)
>
> Yes, I missed another signal_group_exit() in seccomp_set_mode_strict().
> It looks equally unneeded.
>
>> I can't decide which feels cleaner: just letting stuff
>> clean up naturally on death or to short-circuit after taking
>> sighand->siglock.
>
> I'd prefer to simply remove the singal_group_exit checks.
>
> I won't argue if you prefer to keep them, but then please add a comment
> to explain that this is not needed for correctness.
>
> Because otherwise the code looks confusing, as if there is a subtle reason
> why we must not do this if killed.
Sounds good! I'll clean it all up for v10.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v9 09/11] seccomp: introduce writer locking
From: Kees Cook @ 2014-07-10 16:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-arch, linux-mips, Will Drewry, linux-security-module,
Linux API, x86@kernel.org, LKML, Andy Lutomirski, Daniel Borkmann,
Julien Tinnes, Michael Kerrisk (man-pages), Andrew Morton,
David Drysdale, linux-arm-kernel@lists.infradead.org,
Alexei Starovoitov
In-Reply-To: <20140710152418.GB20861@redhat.com>
On Thu, Jul 10, 2014 at 8:24 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/10, Kees Cook wrote:
>>
>> On Wed, Jul 9, 2014 at 11:55 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 07/09, Oleg Nesterov wrote:
>> >>
>> >> On 06/27, Kees Cook wrote:
>> >> >
>> >> > static u32 seccomp_run_filters(int syscall)
>> >> > {
>> >> > - struct seccomp_filter *f;
>> >> > + struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);
>> >>
>> >> I am not sure...
>> >>
>> >> This is fine if this ->filter is the 1st (and only) one, in this case
>> >> we can rely on rmb() in the caller.
>> >>
>> >> But the new filter can be installed at any moment. Say, right after that
>> >> rmb() although this doesn't matter. Either we need smp_read_barrier_depends()
>> >> after that, or smp_load_acquire() like the previous version did?
>> >
>> > Wait... and it seems that seccomp_sync_threads() needs smp_store_release()
>> > when it sets thread->filter = current->filter by the same reason?
>> >
>> > OTOH. smp_store_release() in seccomp_attach_filter() can die, "current"
>> > doesn't need a barrier to serialize with itself.
>>
>> I have lost track of what you're suggesting to change. :)
>
> Perhaps I am just trying to confuse you and myself ;)
>
> But,
>
>> Since rmb() happens before run_filters, isn't the ACCESS_ONCE
>> sufficient?
>
> Yes. But see above. ACCESS_ONCE is sufficient if we read the first filter
> installed by another thread, in this case rmb() pairs with mb_before_atomic()
> before set_bit(TIF_SECCOMP).
>
> IOW, if this threads sees TIF_SECCOMP, it should also see all modifications
> which were done before set_bit, including the data in ->filter points to.
>
>> We only care that TIF_SECCOMP, mode, and some filter is
>> valid. In a tsync thread race, it's okay to use not use the deepest
>> filter node in the list,
>
> Yes, it is fine if we miss yet another filter which was just installed by
> another thread.
>
> But, unless I missed something, the problem is that we can get this new
> filter.
>
> Just to simplify. Suppose TIF_SECCOMP was set a long ago. This thread
> has a single filter F1 and it enters seccomp_run_filters().
>
> Right before it does ACCESS_ONCE() to read the pointer, another thread
> does seccomp_sync_threads() and sets .filter = F2.
>
> If ACCESS_ONCE() returns F1 - everything is fine. But it can see the new
> pointer F2, and in this case we need a barrier to ensure that, say,
> LOAD(F2->prog) will see all the preceding changes in this memory.
And the rmb() isn't sufficient for that? Is another barrier needed
before assigning the filter pointer to make sure the contents it
points to are flushed?
What's the least time-consuming operation I can use in run_filters?
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v9 09/11] seccomp: introduce writer locking
From: Oleg Nesterov @ 2014-07-10 17:35 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linux-mips, Will Drewry, linux-security-module,
Linux API, x86@kernel.org, LKML, Andy Lutomirski, Daniel Borkmann,
Julien Tinnes, Michael Kerrisk (man-pages), Andrew Morton,
David Drysdale, linux-arm-kernel@lists.infradead.org,
Alexei Starovoitov
In-Reply-To: <CAGXu5jKNUn0OcXPyTmqbHwQ_GPMNTeajyrxpd2xAtzjTRFyhpg@mail.gmail.com>
On 07/10, Kees Cook wrote:
>
> On Thu, Jul 10, 2014 at 8:24 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Just to simplify. Suppose TIF_SECCOMP was set a long ago. This thread
> > has a single filter F1 and it enters seccomp_run_filters().
> >
> > Right before it does ACCESS_ONCE() to read the pointer, another thread
> > does seccomp_sync_threads() and sets .filter = F2.
> >
> > If ACCESS_ONCE() returns F1 - everything is fine. But it can see the new
> > pointer F2, and in this case we need a barrier to ensure that, say,
> > LOAD(F2->prog) will see all the preceding changes in this memory.
>
> And the rmb() isn't sufficient for that?
But it has no effect if the pointer was changed _after_ rmb() was already
called.
And, you need a barrier _after_ ACCESS_ONCE().
(Unless, again, we know that this is the first filter, but this is only
by accident).
> Is another barrier needed
> before assigning the filter pointer to make sure the contents it
> points to are flushed?
I think smp_store_release() should be moved from seccomp_attach_filter()
to seccomp_sync_threads(). Although probably it _should_ work either way,
but at least this looks confusing because a) "current" doesn't need a
barrier to serialize wuth itself, and b) it is not clear why it is safe
to change the pointer dereferenced by another thread without a barrier.
> What's the least time-consuming operation I can use in run_filters?
As I said smp_read_barrier_depends() (nop unless alpha) or
smp_load_acquire() which you used in the previous version.
And to remind, afaics smp_load_acquire() in put_filter() should die ;)
Oleg.
^ 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