* Re: [PATCH v3 11/11] nvme: Fix a race condition
From: Christoph Hellwig @ 2016-10-19 13:41 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <2a29b7ec-0113-3450-9a36-b925b47b1fb0@sandisk.com>
Hi Bart,
this looks great!
Reviewed-by: Christoph Hellwig <hch@lst.de>
Some minor nitpicks below:
> void nvme_requeue_req(struct request *req)
> {
> + blk_mq_requeue_request(req, true);
> }
> EXPORT_SYMBOL_GPL(nvme_requeue_req);
Please just remove the nvme_requeue_req wrapper.
>
> @@ -2074,11 +2068,14 @@ EXPORT_SYMBOL_GPL(nvme_kill_queues);
> void nvme_stop_queues(struct nvme_ctrl *ctrl)
> {
> struct nvme_ns *ns;
> + struct request_queue *q;
>
> mutex_lock(&ctrl->namespaces_mutex);
> list_for_each_entry(ns, &ctrl->namespaces, list) {
> + q = ns->queue;
> + blk_mq_cancel_requeue_work(q);
> + blk_mq_stop_hw_queues(q);
> + blk_mq_quiesce_queue(q);
> }
I'd keep the q declaration in the minimal scope, e.g.
list_for_each_entry(ns, &ctrl->namespaces, list) {
struct request_queue *q = ns->queue;
blk_mq_cancel_requeue_work(q);
blk_mq_stop_hw_queues(q);
blk_mq_quiesce_queue(q);
}
^ permalink raw reply
* Re: [PATCH v3 10/11] nvme: Use BLK_MQ_S_STOPPED instead of QUEUE_FLAG_STOPPED in blk-mq code
From: Christoph Hellwig @ 2016-10-19 13:39 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <39c07429-2508-4123-8fe1-ecf407f8571d@sandisk.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v3 09/11] SRP transport, scsi-mq: Wait for .queue_rq() if necessary
From: Christoph Hellwig @ 2016-10-19 13:39 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <23d3615f-6905-0b60-a531-4a4d2cd9cf3f@sandisk.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v3 08/11] SRP transport: Move queuecommand() wait code to SCSI core
From: Christoph Hellwig @ 2016-10-19 13:38 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman,
linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <09a8efcb-d0e0-d5bd-288c-686b553e6326-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Looks fine,
Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 v3 07/11] dm: Fix a race condition related to stopping and starting queues
From: Christoph Hellwig @ 2016-10-19 13:30 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <1e89a7d7-5cae-40b1-ce31-09d630e6f33d@sandisk.com>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
FYI, I wonder how many of the blk_mq_stop_hw_queues do not need
the quiesce call. In the long run it might be better to have
blk_mq_stop_hw_queues to stop an quiesce, and have a
__blk_mq_stop_hw_queues variant to just stop.
^ permalink raw reply
* Re: [PATCH v3 06/11] dm: Use BLK_MQ_S_STOPPED instead of QUEUE_FLAG_STOPPED in blk-mq code
From: Christoph Hellwig @ 2016-10-19 13:28 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <e5facfd0-82d5-10cb-712b-13d7950b78b8@sandisk.com>
This looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Oct 18, 2016 at 02:51:33PM -0700, Bart Van Assche wrote:
> static void dm_mq_start_queue(struct request_queue *q)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(q->queue_lock, flags);
> - queue_flag_clear(QUEUE_FLAG_STOPPED, q);
> - spin_unlock_irqrestore(q->queue_lock, flags);
> -
> blk_mq_start_stopped_hw_queues(q, true);
> blk_mq_kick_requeue_list(q);
FYI, I'm tempted to say we should always call blk_mq_kick_requeue_list
from blk_mq_start_stopped_hw_queues, but that's a separate issue.
^ permalink raw reply
* Re: [PATCH v3 05/11] blk-mq: Add a kick_requeue_list argument to blk_mq_requeue_request()
From: Christoph Hellwig @ 2016-10-19 13:23 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <7b52319b-03f7-e432-909a-2f46232770c6@sandisk.com>
On Tue, Oct 18, 2016 at 02:51:02PM -0700, Bart Van Assche wrote:
> Most blk_mq_requeue_request() and blk_mq_add_to_requeue_list() calls
> are followed by kicking the requeue list. Hence add an argument to
> these two functions that allows to kick the requeue list. This was
> proposed by Christoph Hellwig.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Thanks Bart, this looks fine to me:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v3 04/11] blk-mq: Introduce blk_mq_quiesce_queue()
From: Christoph Hellwig @ 2016-10-19 13:23 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <e42a952c-f245-eb39-d0a1-0336035573f9@sandisk.com>
> +/**
> + * blk_mq_quiesce_queue() - wait until all ongoing queue_rq calls have finished
> + *
> + * Note: this function does not prevent that the struct request end_io()
> + * callback function is invoked. Additionally, it is not prevented that
> + * new queue_rq() calls occur unless the queue has been stopped first.
> + */
> +void blk_mq_quiesce_queue(struct request_queue *q)
If this is intended to be a kerneldoc comment you need to document the 'q'
parameter. If not you should drop the magic "/**" marker.
> +static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
> +{
> + int srcu_idx;
> +
> + WARN_ON(!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask) &&
> + cpu_online(hctx->next_cpu));
> +
> + if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
> + rcu_read_lock();
> + blk_mq_process_rq_list(hctx);
> + rcu_read_unlock();
> + } else {
> + srcu_idx = srcu_read_lock(&hctx->queue_rq_srcu);
> + blk_mq_process_rq_list(hctx);
> + srcu_read_unlock(&hctx->queue_rq_srcu, srcu_idx);
> + }
> +}
Can you document these synchronization changes in detail in the changelog?
> +static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
> + struct request *rq, blk_qc_t *cookie)
> +{
> + if (blk_mq_hctx_stopped(hctx) ||
> + blk_mq_direct_issue_request(rq, cookie) != 0)
> + blk_mq_insert_request(rq, false, true, true);
> +}
Any reason not to merge this function with blk_mq_direct_issue_request?
Otherwise this change looks fine to me.
^ permalink raw reply
* Re: [PATCH v3 03/11] blk-mq: Introduce blk_mq_queue_stopped()
From: Christoph Hellwig @ 2016-10-19 13:19 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman,
linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <b4e8cd03-0654-3e62-a559-ecc996676807-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Looks good,
Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 v3 02/11] blk-mq: Introduce blk_mq_hctx_stopped()
From: Christoph Hellwig @ 2016-10-19 13:19 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <b5b0ffcc-767d-1b90-9c5a-52c57690dea8@sandisk.com>
On Tue, Oct 18, 2016 at 02:49:09PM -0700, Bart Van Assche wrote:
> Multiple functions test the BLK_MQ_S_STOPPED bit so introduce
> a helper function that performs this test.
Looks sensible. Any reason to have it in the public blk-mq.h instead
of the private one, though? I see that dm is using it with this patch,
but that usage should go away once your full series is merged, right?
^ permalink raw reply
* Re: [PATCH v3 01/11] blk-mq: Do not invoke .queue_rq() for a stopped queue
From: Christoph Hellwig @ 2016-10-19 13:17 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Christoph Hellwig, James Bottomley,
Martin K. Petersen, Mike Snitzer, Doug Ledford, Keith Busch,
Ming Lin, Laurence Oberman, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvme@lists.infradead.org
In-Reply-To: <595d4b59-3892-ef24-ef91-b7cab6611af7@sandisk.com>
Good catch,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* [PATCH 2/2] IB/rdmavt: Handle the kthread worker using the new API
From: Petr Mladek @ 2016-10-19 12:07 UTC (permalink / raw)
To: Doug Ledford, Sean Hefty
Cc: Dennis Dalessandro, Hal Rosenstock, linux-rdma, Tejun Heo,
linux-kernel, Petr Mladek
In-Reply-To: <1476878840-14548-1-git-send-email-pmladek@suse.com>
Use the new API to create and destroy the cq kthread worker.
The API hides some implementation details.
In particular, kthread_create_worker() allocates and initializes
struct kthread_worker. It runs the kthread the right way and stores
task_struct into the worker structure. In addition, the *on_cpu()
variant binds the kthread to the given cpu and the related memory
node.
kthread_destroy_worker() flushes all pending works, stops
the kthread and frees the structure.
This patch does not change the existing behavior. Note that we must
use the on_cpu() variant because the function starts the kthread
and it must bind it to the right CPU before waking. The numa node
is associated for given CPU as well.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
drivers/infiniband/sw/rdmavt/cq.c | 34 +++++++++++-----------------------
1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
index 223ec4589fc7..4d0b6992e847 100644
--- a/drivers/infiniband/sw/rdmavt/cq.c
+++ b/drivers/infiniband/sw/rdmavt/cq.c
@@ -503,33 +503,23 @@ int rvt_poll_cq(struct ib_cq *ibcq, int num_entries, struct ib_wc *entry)
*/
int rvt_driver_cq_init(struct rvt_dev_info *rdi)
{
- int ret = 0;
int cpu;
- struct task_struct *task;
+ struct kthread_worker *worker;
if (rdi->worker)
return 0;
+
spin_lock_init(&rdi->n_cqs_lock);
- rdi->worker = kzalloc(sizeof(*rdi->worker), GFP_KERNEL);
- if (!rdi->worker)
- return -ENOMEM;
- kthread_init_worker(rdi->worker);
- task = kthread_create_on_node(
- kthread_worker_fn,
- rdi->worker,
- rdi->dparms.node,
- "%s", rdi->dparms.cq_name);
- if (IS_ERR(task)) {
- kfree(rdi->worker);
- rdi->worker = NULL;
- return PTR_ERR(task);
- }
- set_user_nice(task, MIN_NICE);
cpu = cpumask_first(cpumask_of_node(rdi->dparms.node));
- kthread_bind(task, cpu);
- wake_up_process(task);
- return ret;
+ worker = kthread_create_worker_on_cpu(cpu, 0,
+ "%s", rdi->dparms.cq_name);
+ if (IS_ERR(worker))
+ return PTR_ERR(worker);
+
+ set_user_nice(worker->task, MIN_NICE);
+ rdi->worker = worker;
+ return 0;
}
/**
@@ -549,7 +539,5 @@ void rvt_cq_exit(struct rvt_dev_info *rdi)
rdi->worker = NULL;
spin_unlock_irq(&rdi->n_cqs_lock);
- kthread_flush_worker(worker);
- kthread_stop(worker->task);
- kfree(worker);
+ kthread_destroy_worker(worker);
}
--
1.8.5.6
^ permalink raw reply related
* [PATCH 1/2] IB/rdmavt: Avoid queuing work into a destroyed cq kthread worker
From: Petr Mladek @ 2016-10-19 12:07 UTC (permalink / raw)
To: Doug Ledford, Sean Hefty
Cc: Dennis Dalessandro, Hal Rosenstock, linux-rdma, Tejun Heo,
linux-kernel, Petr Mladek
In-Reply-To: <1476878840-14548-1-git-send-email-pmladek@suse.com>
The memory barrier is not enough to protect queuing works into
a destroyed cq kthread. Just imagine the following situation:
CPU1 CPU2
rvt_cq_enter()
worker = cq->rdi->worker;
rvt_cq_exit()
rdi->worker = NULL;
smp_wmb();
kthread_flush_worker(worker);
kthread_stop(worker->task);
kfree(worker);
// nothing queued yet =>
// nothing flushed and
// happily stopped and freed
if (likely(worker)) {
// true => read before CPU2 acted
cq->notify = RVT_CQ_NONE;
cq->triggered++;
kthread_queue_work(worker, &cq->comptask);
BANG: worker has been flushed/stopped/freed in the meantime.
This patch solves this by protecting the critical sections by
rdi->n_cqs_lock. It seems that this lock is not much contended
and looks reasonable for this purpose.
One catch is that rvt_cq_enter() might be called from IRQ context.
Therefore we must always take the lock with IRQs disabled to avoid
a possible deadlock.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
drivers/infiniband/sw/rdmavt/cq.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
index 6d9904a4a0ab..223ec4589fc7 100644
--- a/drivers/infiniband/sw/rdmavt/cq.c
+++ b/drivers/infiniband/sw/rdmavt/cq.c
@@ -119,18 +119,17 @@ void rvt_cq_enter(struct rvt_cq *cq, struct ib_wc *entry, bool solicited)
if (cq->notify == IB_CQ_NEXT_COMP ||
(cq->notify == IB_CQ_SOLICITED &&
(solicited || entry->status != IB_WC_SUCCESS))) {
- struct kthread_worker *worker;
/*
* This will cause send_complete() to be called in
* another thread.
*/
- smp_read_barrier_depends(); /* see rvt_cq_exit */
- worker = cq->rdi->worker;
- if (likely(worker)) {
+ spin_lock(&cq->rdi->n_cqs_lock);
+ if (likely(cq->rdi->worker)) {
cq->notify = RVT_CQ_NONE;
cq->triggered++;
- kthread_queue_work(worker, &cq->comptask);
+ kthread_queue_work(cq->rdi->worker, &cq->comptask);
}
+ spin_unlock(&cq->rdi->n_cqs_lock);
}
spin_unlock_irqrestore(&cq->lock, flags);
@@ -240,15 +239,15 @@ struct ib_cq *rvt_create_cq(struct ib_device *ibdev,
}
}
- spin_lock(&rdi->n_cqs_lock);
+ spin_lock_irq(&rdi->n_cqs_lock);
if (rdi->n_cqs_allocated == rdi->dparms.props.max_cq) {
- spin_unlock(&rdi->n_cqs_lock);
+ spin_unlock_irq(&rdi->n_cqs_lock);
ret = ERR_PTR(-ENOMEM);
goto bail_ip;
}
rdi->n_cqs_allocated++;
- spin_unlock(&rdi->n_cqs_lock);
+ spin_unlock_irq(&rdi->n_cqs_lock);
if (cq->ip) {
spin_lock_irq(&rdi->pending_lock);
@@ -296,9 +295,9 @@ int rvt_destroy_cq(struct ib_cq *ibcq)
struct rvt_dev_info *rdi = cq->rdi;
kthread_flush_work(&cq->comptask);
- spin_lock(&rdi->n_cqs_lock);
+ spin_lock_irq(&rdi->n_cqs_lock);
rdi->n_cqs_allocated--;
- spin_unlock(&rdi->n_cqs_lock);
+ spin_unlock_irq(&rdi->n_cqs_lock);
if (cq->ip)
kref_put(&cq->ip->ref, rvt_release_mmap_info);
else
@@ -541,12 +540,15 @@ void rvt_cq_exit(struct rvt_dev_info *rdi)
{
struct kthread_worker *worker;
- worker = rdi->worker;
- if (!worker)
+ /* block future queuing from send_complete() */
+ spin_lock_irq(&rdi->n_cqs_lock);
+ if (!rdi->worker) {
+ spin_unlock_irq(&rdi->n_cqs_lock);
return;
- /* blocks future queuing from send_complete() */
+ }
rdi->worker = NULL;
- smp_wmb(); /* See rdi_cq_enter */
+ spin_unlock_irq(&rdi->n_cqs_lock);
+
kthread_flush_worker(worker);
kthread_stop(worker->task);
kfree(worker);
--
1.8.5.6
^ permalink raw reply related
* [PATCH 0/2] IB/rdmavt: cq ktrhead worker fix and API update
From: Petr Mladek @ 2016-10-19 12:07 UTC (permalink / raw)
To: Doug Ledford, Sean Hefty
Cc: Dennis Dalessandro, Hal Rosenstock, linux-rdma, Tejun Heo,
linux-kernel, Petr Mladek
The kthread worker API has been improved in 4.9-rc1. The 2nd
patch uses the new functions and simplifies the kthread worker
creation and destroying.
I have found a possible race when working on the API conversion.
A proposed fix is in the 1st patch.
Both changes are compile tested only. I did not find an easy way
how to test them at runtime.
Petr Mladek (2):
IB/rdmavt: Avoid queuing work into a destroyed cq kthread worker
IB/rdmavt: Handle the kthread worker using the new API
drivers/infiniband/sw/rdmavt/cq.c | 64 +++++++++++++++++----------------------
1 file changed, 27 insertions(+), 37 deletions(-)
--
1.8.5.6
^ permalink raw reply
* Re: [PATCH 10/10] mm: replace access_process_vm() write parameter with gup_flags
From: Michael Ellerman @ 2016-10-19 11:10 UTC (permalink / raw)
To: linux-mm
Cc: linux-mips, linux-fbdev, Jan Kara, kvm, linux-sh, Dave Hansen,
dri-devel, netdev, sparclinux, linux-ia64, linux-s390,
linux-samsung-soc, linux-scsi, linux-rdma, x86, Hugh Dickins,
linux-media, Rik van Riel, intel-gfx, adi-buildroot-devel,
ceph-devel, linux-arm-kernel, Lorenzo Stoakes, linux-cris-kernel,
Linus Torvalds, linuxppc-dev, linux-kernel, linux-security-module,
linux-alpha
In-Reply-To: <20161013002020.3062-11-lstoakes@gmail.com>
Lorenzo Stoakes <lstoakes@gmail.com> writes:
> diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c
> index f52b7db3..010b7b3 100644
> --- a/arch/powerpc/kernel/ptrace32.c
> +++ b/arch/powerpc/kernel/ptrace32.c
> @@ -74,7 +74,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> break;
>
> copied = access_process_vm(child, (u64)addrOthers, &tmp,
> - sizeof(tmp), 0);
> + sizeof(tmp), FOLL_FORCE);
> if (copied != sizeof(tmp))
> break;
> ret = put_user(tmp, (u32 __user *)data);
LGTM.
> @@ -179,7 +179,8 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> break;
> ret = 0;
> if (access_process_vm(child, (u64)addrOthers, &tmp,
> - sizeof(tmp), 1) == sizeof(tmp))
> + sizeof(tmp),
> + FOLL_FORCE | FOLL_WRITE) == sizeof(tmp))
> break;
> ret = -EIO;
> break;
If you're respinning this anyway, can you format that as:
if (access_process_vm(child, (u64)addrOthers, &tmp, sizeof(tmp),
FOLL_FORCE | FOLL_WRITE) == sizeof(tmp))
break;
I realise you probably deliberately didn't do that to make the diff clearer.
Either way:
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
--
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: [PATCHv12 0/3] rdmacg: IB/core: rdma controller support
From: Parav Pandit @ 2016-10-19 9:34 UTC (permalink / raw)
To: Tejun Heo
Cc: Leon Romanovsky, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-rdma,
Li Zefan, Johannes Weiner, Doug Ledford, Christoph Hellwig,
Liran Liss, Hefty, Sean, Jason Gunthorpe, Haggai Eran,
james.l.morris-QHcLZuEGTsvQT0dZR+AlfA, Or Gerlitz, Matan Barak
In-Reply-To: <20161018215134.GB2761-piEFEHQLUPpN0TnZuCh8vA@public.gmane.org>
Hi Tejun,
On Wed, Oct 19, 2016 at 3:21 AM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Hello,
>
> On Wed, Oct 19, 2016 at 01:32:01AM +0530, Parav Pandit wrote:
>> On Fri, Oct 14, 2016 at 4:44 AM, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > I think what you want is just a way to specify absolute limits using
>> > percentages of what's available at the time of configuration -
>> > e.g. being able to say "allow upto 30% of what's available in the
>> > parent".
>>
>> Yes. I am concerned about how to configure < 1% value to avoid
>> floating point math in kernel as thats discouraged.
>> Configuring in range of 1 to 100% for a given resource limits to only
>> 100 or less cgroup instances which I think is not desired.
>
> Heh, we can go for per-mil and use %0 as the suffix if absolutely
> necessary but is this a real issue?
>
We need to provide current version to exceed the 100 containers limit first. :-)
Please let me know if such user interface already exist as reference point.
I prefer to avoid introducing such configuration.
>> > If so, the simplest way would be simply updating the
>> > existing knobs to accept % inputs in addition to absolute values on
>> > writes.
>>
>> I was not sure to overload rdma.max file for accepting % inputs as
>> thats not done in other cgroups. So I was thinking more of weights
>> interface which avoids floating point problem and also allows much
>> wider configuration range.
>
> I think it's a lot more consistent to implement all absoulte limits
> through max.
I agree. I will implement reporting actual max values instead of
reporting "max" string so that percentage configuration can be done by
the user space tools.
(post merging of v12).
Waiting for direction from Doug.
> weight is for actual proportional control which this
> isn't. It's just a fancy way of specifying absolute limits.
>
> Thanks.
>
> --
> tejun
^ permalink raw reply
* Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
From: Michal Hocko @ 2016-10-19 9:23 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Jan Kara, linux-mm, Linus Torvalds, Hugh Dickins, Dave Hansen,
Rik van Riel, Mel Gorman, Andrew Morton, adi-buildroot-devel,
ceph-devel, dri-devel, intel-gfx, kvm, linux-alpha,
linux-arm-kernel, linux-cris-kernel, linux-fbdev, linux-fsdevel,
linux-ia64, linux-kernel, linux-media, linux-mips, linux-rdma,
linux-s390, linux-samsung-soc, linux-scsi, linux-security-module,
linux-sh
In-Reply-To: <20161019090646.GA24243@lucifer>
On Wed 19-10-16 10:06:46, Lorenzo Stoakes wrote:
> On Wed, Oct 19, 2016 at 10:52:05AM +0200, Michal Hocko wrote:
> > yes this is the desirable and expected behavior.
> >
> > > wonder if this is desirable behaviour or whether this ought to be limited to
> > > ptrace system calls. Regardless, by making the flag more visible it makes it
> > > easier to see that this is happening.
> >
> > mem_open already enforces PTRACE_MODE_ATTACH
>
> Ah I missed this, that makes a lot of sense, thanks!
>
> I still wonder whether other invocations of access_remote_vm() in fs/proc/base.c
> (the principle caller of this function) need FOLL_FORCE, for example the various
> calls that simply read data from other processes, so I think the point stands
> about keeping this explicit.
I do agree. Making them explicit will help to clean them up later,
should there be a need.
--
Michal Hocko
SUSE Labs
--
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 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
From: Michal Hocko @ 2016-10-19 9:07 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: linux-mm, Linus Torvalds, Jan Kara, Hugh Dickins, Dave Hansen,
Rik van Riel, Mel Gorman, Andrew Morton, adi-buildroot-devel,
ceph-devel, dri-devel, intel-gfx, kvm, linux-alpha,
linux-arm-kernel, linux-cris-kernel, linux-fbdev, linux-fsdevel,
linux-ia64, linux-kernel, linux-media, linux-mips, linux-rdma,
linux-s390, linux-samsung-soc, linux-scsi, linux-security-module,
linux-sh@
In-Reply-To: <20161019085815.GA22239@lucifer>
On Wed 19-10-16 09:58:15, Lorenzo Stoakes wrote:
> On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> > I am wondering whether we can go further. E.g. it is not really clear to
> > me whether we need an explicit FOLL_REMOTE when we can in fact check
> > mm != current->mm and imply that. Maybe there are some contexts which
> > wouldn't work, I haven't checked.
>
> This flag is set even when /proc/self/mem is used. I've not looked deeply into
> this flag but perhaps accessing your own memory this way can be considered
> 'remote' since you're not accessing it directly. On the other hand, perhaps this
> is just mistaken in this case?
My understanding of the flag is quite limited as well. All I know it is
related to protection keys and it is needed to bypass protection check.
See arch_vma_access_permitted. See also 1b2ee1266ea6 ("mm/core: Do not
enforce PKEY permissions on remote mm access").
--
Michal Hocko
SUSE Labs
--
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 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
From: Lorenzo Stoakes @ 2016-10-19 9:06 UTC (permalink / raw)
To: Michal Hocko
Cc: Jan Kara, linux-mm, Linus Torvalds, Hugh Dickins, Dave Hansen,
Rik van Riel, Mel Gorman, Andrew Morton, adi-buildroot-devel,
ceph-devel, dri-devel, intel-gfx, kvm, linux-alpha,
linux-arm-kernel, linux-cris-kernel, linux-fbdev, linux-fsdevel,
linux-ia64, linux-kernel, linux-media, linux-mips, linux-rdma,
linux-s390, linux-samsung-soc, linux-scsi, linux-security-module,
linux-sh
In-Reply-To: <20161019085204.GD7517@dhcp22.suse.cz>
On Wed, Oct 19, 2016 at 10:52:05AM +0200, Michal Hocko wrote:
> yes this is the desirable and expected behavior.
>
> > wonder if this is desirable behaviour or whether this ought to be limited to
> > ptrace system calls. Regardless, by making the flag more visible it makes it
> > easier to see that this is happening.
>
> mem_open already enforces PTRACE_MODE_ATTACH
Ah I missed this, that makes a lot of sense, thanks!
I still wonder whether other invocations of access_remote_vm() in fs/proc/base.c
(the principle caller of this function) need FOLL_FORCE, for example the various
calls that simply read data from other processes, so I think the point stands
about keeping this explicit.
--
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 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
From: Lorenzo Stoakes @ 2016-10-19 8:58 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mips, linux-fbdev, Jan Kara, kvm, linux-sh, Dave Hansen,
dri-devel, linux-mm, netdev, sparclinux, linux-ia64, linux-s390,
linux-samsung-soc, linux-scsi, linux-rdma, x86, Hugh Dickins,
linux-media, Rik van Riel, intel-gfx, adi-buildroot-devel,
ceph-devel, linux-arm-kernel, linux-cris-kernel, Linus Torvalds,
linuxppc-dev, linux-kernel, linux-security-module, linux-alpha
In-Reply-To: <20161018153050.GC13117@dhcp22.suse.cz>
On Tue, Oct 18, 2016 at 05:30:50PM +0200, Michal Hocko wrote:
> I am wondering whether we can go further. E.g. it is not really clear to
> me whether we need an explicit FOLL_REMOTE when we can in fact check
> mm != current->mm and imply that. Maybe there are some contexts which
> wouldn't work, I haven't checked.
This flag is set even when /proc/self/mem is used. I've not looked deeply into
this flag but perhaps accessing your own memory this way can be considered
'remote' since you're not accessing it directly. On the other hand, perhaps this
is just mistaken in this case?
> I guess there is more work in that area and I do not want to impose all
> that work on you, but I couldn't resist once I saw you playing in that
> area ;) Definitely a good start!
Thanks, I am more than happy to go as far down this rabbit hole as is helpful,
no imposition at all :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
From: Michal Hocko @ 2016-10-19 8:52 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Jan Kara, linux-mm, Linus Torvalds, Hugh Dickins, Dave Hansen,
Rik van Riel, Mel Gorman, Andrew Morton, adi-buildroot-devel,
ceph-devel, dri-devel, intel-gfx, kvm, linux-alpha,
linux-arm-kernel, linux-cris-kernel, linux-fbdev, linux-fsdevel,
linux-ia64, linux-kernel, linux-media, linux-mips, linux-rdma,
linux-s390, linux-samsung-soc, linux-scsi, linux-security-module,
linux-sh
In-Reply-To: <20161019084045.GA19441@lucifer>
On Wed 19-10-16 09:40:45, Lorenzo Stoakes wrote:
> On Wed, Oct 19, 2016 at 10:13:52AM +0200, Michal Hocko wrote:
> > On Wed 19-10-16 09:59:03, Jan Kara wrote:
> > > On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote:
> > > > This patch removes the write parameter from __access_remote_vm() and replaces it
> > > > with a gup_flags parameter as use of this function previously _implied_
> > > > FOLL_FORCE, whereas after this patch callers explicitly pass this flag.
> > > >
> > > > We make this explicit as use of FOLL_FORCE can result in surprising behaviour
> > > > (and hence bugs) within the mm subsystem.
> > > >
> > > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > >
> > > So I'm not convinced this (and the following two patches) is actually
> > > helping much. By grepping for FOLL_FORCE we will easily see that any caller
> > > of access_remote_vm() gets that semantics and can thus continue search
> >
> > I am really wondering. Is there anything inherent that would require
> > FOLL_FORCE for access_remote_vm? I mean FOLL_FORCE is a really
> > non-trivial thing. It doesn't obey vma permissions so we should really
> > minimize its usage. Do all of those users really need FOLL_FORCE?
>
> I wonder about this also, for example by accessing /proc/self/mem you trigger
> access_remote_vm() and consequently get_user_pages_remote() meaning FOLL_FORCE
> is implied and you can use /proc/self/mem to override any VMA permissions. I
yes this is the desirable and expected behavior.
> wonder if this is desirable behaviour or whether this ought to be limited to
> ptrace system calls. Regardless, by making the flag more visible it makes it
> easier to see that this is happening.
mem_open already enforces PTRACE_MODE_ATTACH
--
Michal Hocko
SUSE Labs
--
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 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
From: Lorenzo Stoakes @ 2016-10-19 8:40 UTC (permalink / raw)
To: Michal Hocko
Cc: Jan Kara, linux-mm, Linus Torvalds, Hugh Dickins, Dave Hansen,
Rik van Riel, Mel Gorman, Andrew Morton, adi-buildroot-devel,
ceph-devel, dri-devel, intel-gfx, kvm, linux-alpha,
linux-arm-kernel, linux-cris-kernel, linux-fbdev, linux-fsdevel,
linux-ia64, linux-kernel, linux-media, linux-mips, linux-rdma,
linux-s390, linux-samsung-soc, linux-scsi, linux-security-module,
linux-sh
In-Reply-To: <20161019081352.GB7562@dhcp22.suse.cz>
On Wed, Oct 19, 2016 at 10:13:52AM +0200, Michal Hocko wrote:
> On Wed 19-10-16 09:59:03, Jan Kara wrote:
> > On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote:
> > > This patch removes the write parameter from __access_remote_vm() and replaces it
> > > with a gup_flags parameter as use of this function previously _implied_
> > > FOLL_FORCE, whereas after this patch callers explicitly pass this flag.
> > >
> > > We make this explicit as use of FOLL_FORCE can result in surprising behaviour
> > > (and hence bugs) within the mm subsystem.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> >
> > So I'm not convinced this (and the following two patches) is actually
> > helping much. By grepping for FOLL_FORCE we will easily see that any caller
> > of access_remote_vm() gets that semantics and can thus continue search
>
> I am really wondering. Is there anything inherent that would require
> FOLL_FORCE for access_remote_vm? I mean FOLL_FORCE is a really
> non-trivial thing. It doesn't obey vma permissions so we should really
> minimize its usage. Do all of those users really need FOLL_FORCE?
I wonder about this also, for example by accessing /proc/self/mem you trigger
access_remote_vm() and consequently get_user_pages_remote() meaning FOLL_FORCE
is implied and you can use /proc/self/mem to override any VMA permissions. I
wonder if this is desirable behaviour or whether this ought to be limited to
ptrace system calls. Regardless, by making the flag more visible it makes it
easier to see that this is happening.
Note that this /proc/self/mem behaviour is what triggered the bug that brought
about this discussion in the first place -
https://marc.info/?l=linux-mm&m=147363447105059 - as using /proc/self/mem this
way on PROT_NONE memory broke some assumptions.
On the other hand I see your point Jan in that you know any caller of these
functions will have FOLL_FORCE implied, and you have to decide to stop passing
the flag at _some_ point in the stack, however it seems fairly sane to have that
stopping point exist outside of exported gup functions so the gup interface
forces explicitness but callers can wrap things as they need.
--
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 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
From: Michal Hocko @ 2016-10-19 8:13 UTC (permalink / raw)
To: Jan Kara
Cc: linux-mips, linux-fbdev, linux-ia64, kvm, linux-sh, Dave Hansen,
dri-devel, linux-mm, netdev, sparclinux, linux-s390,
linux-samsung-soc, linux-scsi, linux-rdma, x86, Hugh Dickins,
linux-media, Rik van Riel, intel-gfx, adi-buildroot-devel,
ceph-devel, linux-arm-kernel, Lorenzo Stoakes, linux-cris-kernel,
Linus Torvalds, linuxppc-dev, linux-kernel, linux-security-module,
linux-alpha
In-Reply-To: <20161019075903.GP29967@quack2.suse.cz>
On Wed 19-10-16 09:59:03, Jan Kara wrote:
> On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote:
> > This patch removes the write parameter from __access_remote_vm() and replaces it
> > with a gup_flags parameter as use of this function previously _implied_
> > FOLL_FORCE, whereas after this patch callers explicitly pass this flag.
> >
> > We make this explicit as use of FOLL_FORCE can result in surprising behaviour
> > (and hence bugs) within the mm subsystem.
> >
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
>
> So I'm not convinced this (and the following two patches) is actually
> helping much. By grepping for FOLL_FORCE we will easily see that any caller
> of access_remote_vm() gets that semantics and can thus continue search
I am really wondering. Is there anything inherent that would require
FOLL_FORCE for access_remote_vm? I mean FOLL_FORCE is a really
non-trivial thing. It doesn't obey vma permissions so we should really
minimize its usage. Do all of those users really need FOLL_FORCE?
Anyway I would rather see the flag explicit and used at more places than
hidden behind a helper function.
--
Michal Hocko
SUSE Labs
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
From: Jan Kara @ 2016-10-19 7:59 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: linux-mm, Linus Torvalds, Jan Kara, Hugh Dickins, Dave Hansen,
Rik van Riel, Mel Gorman, Andrew Morton, adi-buildroot-devel,
ceph-devel, dri-devel, intel-gfx, kvm, linux-alpha,
linux-arm-kernel, linux-cris-kernel, linux-fbdev, linux-fsdevel,
linux-ia64, linux-kernel, linux-media, linux-mips, linux-rdma,
linux-s390, linux-samsung-soc, linux-scsi, linux-security-module,
linux-sh@
In-Reply-To: <20161013002020.3062-9-lstoakes@gmail.com>
On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote:
> This patch removes the write parameter from __access_remote_vm() and replaces it
> with a gup_flags parameter as use of this function previously _implied_
> FOLL_FORCE, whereas after this patch callers explicitly pass this flag.
>
> We make this explicit as use of FOLL_FORCE can result in surprising behaviour
> (and hence bugs) within the mm subsystem.
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
So I'm not convinced this (and the following two patches) is actually
helping much. By grepping for FOLL_FORCE we will easily see that any caller
of access_remote_vm() gets that semantics and can thus continue search
accordingly (it is much simpler than searching for all get_user_pages()
users and extracting from parameter lists what they actually pass as
'force' argument). Sure it makes somewhat more visible to callers of
access_remote_vm() that they get FOLL_FORCE semantics but OTOH it also
opens a space for issues where a caller of access_remote_vm() actually
wants FOLL_FORCE (and currently all of them want it) and just mistakenly
does not set it. All in all I'd prefer to keep access_remote_vm() and
friends as is...
Honza
> ---
> mm/memory.c | 23 +++++++++++++++--------
> mm/nommu.c | 9 ++++++---
> 2 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 20a9adb..79ebed3 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3869,14 +3869,11 @@ EXPORT_SYMBOL_GPL(generic_access_phys);
> * given task for page fault accounting.
> */
> static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> - unsigned long addr, void *buf, int len, int write)
> + unsigned long addr, void *buf, int len, unsigned int gup_flags)
> {
> struct vm_area_struct *vma;
> void *old_buf = buf;
> - unsigned int flags = FOLL_FORCE;
> -
> - if (write)
> - flags |= FOLL_WRITE;
> + int write = gup_flags & FOLL_WRITE;
>
> down_read(&mm->mmap_sem);
> /* ignore errors, just check how much was successfully transferred */
> @@ -3886,7 +3883,7 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> struct page *page = NULL;
>
> ret = get_user_pages_remote(tsk, mm, addr, 1,
> - flags, &page, &vma);
> + gup_flags, &page, &vma);
> if (ret <= 0) {
> #ifndef CONFIG_HAVE_IOREMAP_PROT
> break;
> @@ -3945,7 +3942,12 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> int access_remote_vm(struct mm_struct *mm, unsigned long addr,
> void *buf, int len, int write)
> {
> - return __access_remote_vm(NULL, mm, addr, buf, len, write);
> + unsigned int flags = FOLL_FORCE;
> +
> + if (write)
> + flags |= FOLL_WRITE;
> +
> + return __access_remote_vm(NULL, mm, addr, buf, len, flags);
> }
>
> /*
> @@ -3958,12 +3960,17 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr,
> {
> struct mm_struct *mm;
> int ret;
> + unsigned int flags = FOLL_FORCE;
>
> mm = get_task_mm(tsk);
> if (!mm)
> return 0;
>
> - ret = __access_remote_vm(tsk, mm, addr, buf, len, write);
> + if (write)
> + flags |= FOLL_WRITE;
> +
> + ret = __access_remote_vm(tsk, mm, addr, buf, len, flags);
> +
> mmput(mm);
>
> return ret;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 70cb844..bde7df3 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1809,9 +1809,10 @@ void filemap_map_pages(struct fault_env *fe,
> EXPORT_SYMBOL(filemap_map_pages);
>
> static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> - unsigned long addr, void *buf, int len, int write)
> + unsigned long addr, void *buf, int len, unsigned int gup_flags)
> {
> struct vm_area_struct *vma;
> + int write = gup_flags & FOLL_WRITE;
>
> down_read(&mm->mmap_sem);
>
> @@ -1853,7 +1854,8 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> int access_remote_vm(struct mm_struct *mm, unsigned long addr,
> void *buf, int len, int write)
> {
> - return __access_remote_vm(NULL, mm, addr, buf, len, write);
> + return __access_remote_vm(NULL, mm, addr, buf, len,
> + write ? FOLL_WRITE : 0);
> }
>
> /*
> @@ -1871,7 +1873,8 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in
> if (!mm)
> return 0;
>
> - len = __access_remote_vm(tsk, mm, addr, buf, len, write);
> + len = __access_remote_vm(tsk, mm, addr, buf, len,
> + write ? FOLL_WRITE : 0);
>
> mmput(mm);
> return len;
> --
> 2.10.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
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 07/10] mm: replace get_user_pages_remote() write/force parameters with gup_flags
From: Jan Kara @ 2016-10-19 7:47 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: linux-mips, linux-fbdev, Jan Kara, kvm, linux-sh, Dave Hansen,
dri-devel, linux-mm, netdev, sparclinux, linux-ia64, linux-s390,
linux-samsung-soc, linux-scsi, linux-rdma, x86, Hugh Dickins,
linux-media, Rik van Riel, intel-gfx, adi-buildroot-devel,
ceph-devel, linux-arm-kernel, linux-cris-kernel, Linus Torvalds,
linuxppc-dev, linux-kernel, linux-security-module, linux-alpha
In-Reply-To: <20161013002020.3062-8-lstoakes@gmail.com>
On Thu 13-10-16 01:20:17, Lorenzo Stoakes wrote:
> This patch removes the write and force parameters from get_user_pages_remote()
> and replaces them with a gup_flags parameter to make the use of FOLL_FORCE
> explicit in callers as use of this flag can result in surprising behaviour (and
> hence bugs) within the mm subsystem.
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++++--
> drivers/gpu/drm/i915/i915_gem_userptr.c | 6 +++++-
> drivers/infiniband/core/umem_odp.c | 7 +++++--
> fs/exec.c | 9 +++++++--
> include/linux/mm.h | 2 +-
> kernel/events/uprobes.c | 6 ++++--
> mm/gup.c | 22 +++++++---------------
> mm/memory.c | 6 +++++-
> security/tomoyo/domain.c | 2 +-
> 9 files changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 5ce3603..0370b84 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -748,19 +748,22 @@ static struct page **etnaviv_gem_userptr_do_get_pages(
> int ret = 0, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT;
> struct page **pvec;
> uintptr_t ptr;
> + unsigned int flags = 0;
>
> pvec = drm_malloc_ab(npages, sizeof(struct page *));
> if (!pvec)
> return ERR_PTR(-ENOMEM);
>
> + if (!etnaviv_obj->userptr.ro)
> + flags |= FOLL_WRITE;
> +
> pinned = 0;
> ptr = etnaviv_obj->userptr.ptr;
>
> down_read(&mm->mmap_sem);
> while (pinned < npages) {
> ret = get_user_pages_remote(task, mm, ptr, npages - pinned,
> - !etnaviv_obj->userptr.ro, 0,
> - pvec + pinned, NULL);
> + flags, pvec + pinned, NULL);
> if (ret < 0)
> break;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index e537930..c6f780f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -508,6 +508,10 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
> pvec = drm_malloc_gfp(npages, sizeof(struct page *), GFP_TEMPORARY);
> if (pvec != NULL) {
> struct mm_struct *mm = obj->userptr.mm->mm;
> + unsigned int flags = 0;
> +
> + if (!obj->userptr.read_only)
> + flags |= FOLL_WRITE;
>
> ret = -EFAULT;
> if (atomic_inc_not_zero(&mm->mm_users)) {
> @@ -517,7 +521,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
> (work->task, mm,
> obj->userptr.ptr + pinned * PAGE_SIZE,
> npages - pinned,
> - !obj->userptr.read_only, 0,
> + flags,
> pvec + pinned, NULL);
> if (ret < 0)
> break;
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index 75077a0..1f0fe32 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -527,6 +527,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
> u64 off;
> int j, k, ret = 0, start_idx, npages = 0;
> u64 base_virt_addr;
> + unsigned int flags = 0;
>
> if (access_mask == 0)
> return -EINVAL;
> @@ -556,6 +557,9 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
> goto out_put_task;
> }
>
> + if (access_mask & ODP_WRITE_ALLOWED_BIT)
> + flags |= FOLL_WRITE;
> +
> start_idx = (user_virt - ib_umem_start(umem)) >> PAGE_SHIFT;
> k = start_idx;
>
> @@ -574,8 +578,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt,
> */
> npages = get_user_pages_remote(owning_process, owning_mm,
> user_virt, gup_num_pages,
> - access_mask & ODP_WRITE_ALLOWED_BIT,
> - 0, local_page_list, NULL);
> + flags, local_page_list, NULL);
> up_read(&owning_mm->mmap_sem);
>
> if (npages < 0)
> diff --git a/fs/exec.c b/fs/exec.c
> index 6fcfb3f..4e497b9 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -191,6 +191,7 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
> {
> struct page *page;
> int ret;
> + unsigned int gup_flags = FOLL_FORCE;
>
> #ifdef CONFIG_STACK_GROWSUP
> if (write) {
> @@ -199,12 +200,16 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
> return NULL;
> }
> #endif
> +
> + if (write)
> + gup_flags |= FOLL_WRITE;
> +
> /*
> * We are doing an exec(). 'current' is the process
> * doing the exec and bprm->mm is the new process's mm.
> */
> - ret = get_user_pages_remote(current, bprm->mm, pos, 1, write,
> - 1, &page, NULL);
> + ret = get_user_pages_remote(current, bprm->mm, pos, 1, gup_flags,
> + &page, NULL);
> if (ret <= 0)
> return NULL;
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 686a477..2a481d3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1276,7 +1276,7 @@ long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
> struct vm_area_struct **vmas, int *nonblocking);
> long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm,
> unsigned long start, unsigned long nr_pages,
> - int write, int force, struct page **pages,
> + unsigned int gup_flags, struct page **pages,
> struct vm_area_struct **vmas);
> long get_user_pages(unsigned long start, unsigned long nr_pages,
> unsigned int gup_flags, struct page **pages,
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index d4129bb..f9ec9ad 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -300,7 +300,8 @@ int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr,
>
> retry:
> /* Read the page with vaddr into memory */
> - ret = get_user_pages_remote(NULL, mm, vaddr, 1, 0, 1, &old_page, &vma);
> + ret = get_user_pages_remote(NULL, mm, vaddr, 1, FOLL_FORCE, &old_page,
> + &vma);
> if (ret <= 0)
> return ret;
>
> @@ -1710,7 +1711,8 @@ static int is_trap_at_addr(struct mm_struct *mm, unsigned long vaddr)
> * but we treat this as a 'remote' access since it is
> * essentially a kernel access to the memory.
> */
> - result = get_user_pages_remote(NULL, mm, vaddr, 1, 0, 1, &page, NULL);
> + result = get_user_pages_remote(NULL, mm, vaddr, 1, FOLL_FORCE, &page,
> + NULL);
> if (result < 0)
> return result;
>
> diff --git a/mm/gup.c b/mm/gup.c
> index dc91303..0deecf3 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -905,9 +905,7 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
> * @mm: mm_struct of target mm
> * @start: starting user address
> * @nr_pages: number of pages from start to pin
> - * @write: whether pages will be written to by the caller
> - * @force: whether to force access even when user mapping is currently
> - * protected (but never forces write access to shared mapping).
> + * @gup_flags: flags modifying lookup behaviour
> * @pages: array that receives pointers to the pages pinned.
> * Should be at least nr_pages long. Or NULL, if caller
> * only intends to ensure the pages are faulted in.
> @@ -936,9 +934,9 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
> * or similar operation cannot guarantee anything stronger anyway because
> * locks can't be held over the syscall boundary.
> *
> - * If write=0, the page must not be written to. If the page is written to,
> - * set_page_dirty (or set_page_dirty_lock, as appropriate) must be called
> - * after the page is finished with, and before put_page is called.
> + * If gup_flags & FOLL_WRITE == 0, the page must not be written to. If the page
> + * is written to, set_page_dirty (or set_page_dirty_lock, as appropriate) must
> + * be called after the page is finished with, and before put_page is called.
> *
> * get_user_pages is typically used for fewer-copy IO operations, to get a
> * handle on the memory by some means other than accesses via the user virtual
> @@ -955,18 +953,12 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
> */
> long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm,
> unsigned long start, unsigned long nr_pages,
> - int write, int force, struct page **pages,
> + unsigned int gup_flags, struct page **pages,
> struct vm_area_struct **vmas)
> {
> - unsigned int flags = FOLL_TOUCH | FOLL_REMOTE;
> -
> - if (write)
> - flags |= FOLL_WRITE;
> - if (force)
> - flags |= FOLL_FORCE;
> -
> return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas,
> - NULL, false, flags);
> + NULL, false,
> + gup_flags | FOLL_TOUCH | FOLL_REMOTE);
> }
> EXPORT_SYMBOL(get_user_pages_remote);
>
> diff --git a/mm/memory.c b/mm/memory.c
> index fc1987d..20a9adb 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3873,6 +3873,10 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> {
> struct vm_area_struct *vma;
> void *old_buf = buf;
> + unsigned int flags = FOLL_FORCE;
> +
> + if (write)
> + flags |= FOLL_WRITE;
>
> down_read(&mm->mmap_sem);
> /* ignore errors, just check how much was successfully transferred */
> @@ -3882,7 +3886,7 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> struct page *page = NULL;
>
> ret = get_user_pages_remote(tsk, mm, addr, 1,
> - write, 1, &page, &vma);
> + flags, &page, &vma);
> if (ret <= 0) {
> #ifndef CONFIG_HAVE_IOREMAP_PROT
> break;
> diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
> index ade7c6c..682b73a 100644
> --- a/security/tomoyo/domain.c
> +++ b/security/tomoyo/domain.c
> @@ -881,7 +881,7 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
> * the execve().
> */
> if (get_user_pages_remote(current, bprm->mm, pos, 1,
> - 0, 1, &page, NULL) <= 0)
> + FOLL_FORCE, &page, NULL) <= 0)
> return false;
> #else
> page = bprm->page[pos / PAGE_SIZE];
> --
> 2.10.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ 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