* [PATCH 1/2] xen-blkback: implement safe iterator for the list of persistent grants
@ 2012-12-04 14:21 Roger Pau Monne
2012-12-04 14:21 ` [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry Roger Pau Monne
0 siblings, 1 reply; 7+ messages in thread
From: Roger Pau Monne @ 2012-12-04 14:21 UTC (permalink / raw)
To: linux-kernel; +Cc: Roger Pau Monne, Konrad Rzeszutek Wilk, xen-devel
Change foreach_grant iterator to a safe version, that allows freeing
the element while iterating. Also move the free code in
free_persistent_gnts to prevent freeing the element before the rb_next
call.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: xen-devel@lists.xen.org
---
drivers/block/xen-blkback/blkback.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 74374fb..5ac841f 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -161,10 +161,12 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
static void make_response(struct xen_blkif *blkif, u64 id,
unsigned short op, int st);
-#define foreach_grant(pos, rbtree, node) \
- for ((pos) = container_of(rb_first((rbtree)), typeof(*(pos)), node); \
+#define foreach_grant_safe(pos, n, rbtree, node) \
+ for ((pos) = container_of(rb_first((rbtree)), typeof(*(pos)), node), \
+ (n) = rb_next(&(pos)->node); \
&(pos)->node != NULL; \
- (pos) = container_of(rb_next(&(pos)->node), typeof(*(pos)), node))
+ (pos) = container_of(n, typeof(*(pos)), node), \
+ (n) = (&(pos)->node != NULL) ? rb_next(&(pos)->node) : NULL)
static void add_persistent_gnt(struct rb_root *root,
@@ -217,10 +219,11 @@ static void free_persistent_gnts(struct rb_root *root, unsigned int num)
struct gnttab_unmap_grant_ref unmap[BLKIF_MAX_SEGMENTS_PER_REQUEST];
struct page *pages[BLKIF_MAX_SEGMENTS_PER_REQUEST];
struct persistent_gnt *persistent_gnt;
+ struct rb_node *n;
int ret = 0;
int segs_to_unmap = 0;
- foreach_grant(persistent_gnt, root, node) {
+ foreach_grant_safe(persistent_gnt, n, root, node) {
BUG_ON(persistent_gnt->handle ==
BLKBACK_INVALID_HANDLE);
gnttab_set_unmap_op(&unmap[segs_to_unmap],
@@ -230,9 +233,6 @@ static void free_persistent_gnts(struct rb_root *root, unsigned int num)
persistent_gnt->handle);
pages[segs_to_unmap] = persistent_gnt->page;
- rb_erase(&persistent_gnt->node, root);
- kfree(persistent_gnt);
- num--;
if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST ||
!rb_next(&persistent_gnt->node)) {
@@ -241,6 +241,10 @@ static void free_persistent_gnts(struct rb_root *root, unsigned int num)
BUG_ON(ret);
segs_to_unmap = 0;
}
+
+ rb_erase(&persistent_gnt->node, root);
+ kfree(persistent_gnt);
+ num--;
}
BUG_ON(num != 0);
}
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry
2012-12-04 14:21 [PATCH 1/2] xen-blkback: implement safe iterator for the list of persistent grants Roger Pau Monne
@ 2012-12-04 14:21 ` Roger Pau Monne
2012-12-07 20:20 ` [Xen-devel] " Konrad Rzeszutek Wilk
0 siblings, 1 reply; 7+ messages in thread
From: Roger Pau Monne @ 2012-12-04 14:21 UTC (permalink / raw)
To: linux-kernel; +Cc: Roger Pau Monne, Konrad Rzeszutek Wilk, xen-devel
Implement a safe version of llist_for_each_entry, and use it in
blkif_free. Previously grants where freed while iterating the list,
which lead to dereferences when trying to fetch the next item.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: xen-devel@lists.xen.org
---
drivers/block/xen-blkfront.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 96e9b00..df21b05 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -143,6 +143,13 @@ static DEFINE_SPINLOCK(minor_lock);
#define DEV_NAME "xvd" /* name in /dev */
+#define llist_for_each_entry_safe(pos, n, node, member) \
+ for ((pos) = llist_entry((node), typeof(*(pos)), member), \
+ (n) = (pos)->member.next; \
+ &(pos)->member != NULL; \
+ (pos) = llist_entry(n, typeof(*(pos)), member), \
+ (n) = (&(pos)->member != NULL) ? (pos)->member.next : NULL)
+
static int get_id_from_freelist(struct blkfront_info *info)
{
unsigned long free = info->shadow_free;
@@ -792,6 +799,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
{
struct llist_node *all_gnts;
struct grant *persistent_gnt;
+ struct llist_node *n;
/* Prevent new requests being issued until we fix things up. */
spin_lock_irq(&info->io_lock);
@@ -804,7 +812,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
/* Remove all persistent grants */
if (info->persistent_gnts_c) {
all_gnts = llist_del_all(&info->persistent_gnts);
- llist_for_each_entry(persistent_gnt, all_gnts, node) {
+ llist_for_each_entry_safe(persistent_gnt, n, all_gnts, node) {
gnttab_end_foreign_access(persistent_gnt->gref, 0, 0UL);
__free_page(pfn_to_page(persistent_gnt->pfn));
kfree(persistent_gnt);
--
1.7.7.5 (Apple Git-26)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry
2012-12-04 14:21 ` [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry Roger Pau Monne
@ 2012-12-07 20:20 ` Konrad Rzeszutek Wilk
2012-12-10 12:15 ` Roger Pau Monné
0 siblings, 1 reply; 7+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-12-07 20:20 UTC (permalink / raw)
To: Roger Pau Monne, akpm, sfr, peterz
Cc: linux-kernel, Konrad Rzeszutek Wilk, xen-devel
On Tue, Dec 04, 2012 at 03:21:53PM +0100, Roger Pau Monne wrote:
> Implement a safe version of llist_for_each_entry, and use it in
> blkif_free. Previously grants where freed while iterating the list,
> which lead to dereferences when trying to fetch the next item.
Looks like xen-blkfront is the only user of this llist_for_each_entry.
Would it be more prudent to put the macro in the llist.h file?
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad@kernel.org>
> Cc: xen-devel@lists.xen.org
> ---
> drivers/block/xen-blkfront.c | 10 +++++++++-
> 1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 96e9b00..df21b05 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -143,6 +143,13 @@ static DEFINE_SPINLOCK(minor_lock);
>
> #define DEV_NAME "xvd" /* name in /dev */
>
> +#define llist_for_each_entry_safe(pos, n, node, member) \
> + for ((pos) = llist_entry((node), typeof(*(pos)), member), \
> + (n) = (pos)->member.next; \
> + &(pos)->member != NULL; \
> + (pos) = llist_entry(n, typeof(*(pos)), member), \
> + (n) = (&(pos)->member != NULL) ? (pos)->member.next : NULL)
> +
> static int get_id_from_freelist(struct blkfront_info *info)
> {
> unsigned long free = info->shadow_free;
> @@ -792,6 +799,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
> {
> struct llist_node *all_gnts;
> struct grant *persistent_gnt;
> + struct llist_node *n;
>
> /* Prevent new requests being issued until we fix things up. */
> spin_lock_irq(&info->io_lock);
> @@ -804,7 +812,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
> /* Remove all persistent grants */
> if (info->persistent_gnts_c) {
> all_gnts = llist_del_all(&info->persistent_gnts);
> - llist_for_each_entry(persistent_gnt, all_gnts, node) {
> + llist_for_each_entry_safe(persistent_gnt, n, all_gnts, node) {
> gnttab_end_foreign_access(persistent_gnt->gref, 0, 0UL);
> __free_page(pfn_to_page(persistent_gnt->pfn));
> kfree(persistent_gnt);
> --
> 1.7.7.5 (Apple Git-26)
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry
2012-12-07 20:20 ` [Xen-devel] " Konrad Rzeszutek Wilk
@ 2012-12-10 12:15 ` Roger Pau Monné
2012-12-10 15:15 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 7+ messages in thread
From: Roger Pau Monné @ 2012-12-10 12:15 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: akpm, sfr, peterz, linux-kernel, Konrad Rzeszutek Wilk, xen-devel
On 07/12/12 21:20, Konrad Rzeszutek Wilk wrote:
> On Tue, Dec 04, 2012 at 03:21:53PM +0100, Roger Pau Monne wrote:
>> Implement a safe version of llist_for_each_entry, and use it in
>> blkif_free. Previously grants where freed while iterating the list,
>> which lead to dereferences when trying to fetch the next item.
>
> Looks like xen-blkfront is the only user of this llist_for_each_entry.
>
> Would it be more prudent to put the macro in the llist.h file?
I'm not able to find out who is the maintainer of llist, should I just
CC it's author?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry
2012-12-10 12:15 ` Roger Pau Monné
@ 2012-12-10 15:15 ` Konrad Rzeszutek Wilk
2012-12-10 17:05 ` Roger Pau Monné
0 siblings, 1 reply; 7+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-12-10 15:15 UTC (permalink / raw)
To: Roger Pau Monné
Cc: akpm, sfr, peterz, linux-kernel, Konrad Rzeszutek Wilk, xen-devel
On Mon, Dec 10, 2012 at 01:15:50PM +0100, Roger Pau Monné wrote:
> On 07/12/12 21:20, Konrad Rzeszutek Wilk wrote:
> > On Tue, Dec 04, 2012 at 03:21:53PM +0100, Roger Pau Monne wrote:
> >> Implement a safe version of llist_for_each_entry, and use it in
> >> blkif_free. Previously grants where freed while iterating the list,
> >> which lead to dereferences when trying to fetch the next item.
> >
> > Looks like xen-blkfront is the only user of this llist_for_each_entry.
> >
> > Would it be more prudent to put the macro in the llist.h file?
>
> I'm not able to find out who is the maintainer of llist, should I just
> CC it's author?
Sure. I CC-ed akpm here to solicit his input as well. Either way I am
OK wit this being in xen-blkfront but it just seems that it could
be useful in the llist file since that is where the non-safe version
resides.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry
2012-12-10 15:15 ` Konrad Rzeszutek Wilk
@ 2012-12-10 17:05 ` Roger Pau Monné
2012-12-12 21:01 ` Andrew Morton
0 siblings, 1 reply; 7+ messages in thread
From: Roger Pau Monné @ 2012-12-10 17:05 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: akpm, sfr, peterz, linux-kernel, Konrad Rzeszutek Wilk, xen-devel
On 10/12/12 16:15, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 10, 2012 at 01:15:50PM +0100, Roger Pau Monné wrote:
>> On 07/12/12 21:20, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Dec 04, 2012 at 03:21:53PM +0100, Roger Pau Monne wrote:
>>>> Implement a safe version of llist_for_each_entry, and use it in
>>>> blkif_free. Previously grants where freed while iterating the list,
>>>> which lead to dereferences when trying to fetch the next item.
>>>
>>> Looks like xen-blkfront is the only user of this llist_for_each_entry.
>>>
>>> Would it be more prudent to put the macro in the llist.h file?
>>
>> I'm not able to find out who is the maintainer of llist, should I just
>> CC it's author?
>
> Sure. I CC-ed akpm here to solicit his input as well. Either way I am
> OK wit this being in xen-blkfront but it just seems that it could
> be useful in the llist file since that is where the non-safe version
> resides.
I'm going to resend this series with llist_for_each_entry_safe added to
llist.h in a separated patch. I wouldn't like to delay this series
because we cannot get llist_for_each_entry_safe added to llist.h, that's
why I've added it to blkfront in the first place.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry
2012-12-10 17:05 ` Roger Pau Monné
@ 2012-12-12 21:01 ` Andrew Morton
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Morton @ 2012-12-12 21:01 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Konrad Rzeszutek Wilk, sfr, peterz, linux-kernel,
Konrad Rzeszutek Wilk, xen-devel
On Mon, 10 Dec 2012 18:05:23 +0100
Roger Pau Monné <roger.pau@citrix.com> wrote:
> On 10/12/12 16:15, Konrad Rzeszutek Wilk wrote:
> > On Mon, Dec 10, 2012 at 01:15:50PM +0100, Roger Pau Monné wrote:
> >> On 07/12/12 21:20, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Dec 04, 2012 at 03:21:53PM +0100, Roger Pau Monne wrote:
> >>>> Implement a safe version of llist_for_each_entry, and use it in
> >>>> blkif_free. Previously grants where freed while iterating the list,
> >>>> which lead to dereferences when trying to fetch the next item.
> >>>
> >>> Looks like xen-blkfront is the only user of this llist_for_each_entry.
> >>>
> >>> Would it be more prudent to put the macro in the llist.h file?
> >>
> >> I'm not able to find out who is the maintainer of llist, should I just
> >> CC it's author?
> >
> > Sure. I CC-ed akpm here to solicit his input as well. Either way I am
> > OK wit this being in xen-blkfront but it just seems that it could
> > be useful in the llist file since that is where the non-safe version
> > resides.
>
> I'm going to resend this series with llist_for_each_entry_safe added to
> llist.h in a separated patch. I wouldn't like to delay this series
> because we cannot get llist_for_each_entry_safe added to llist.h, that's
> why I've added it to blkfront in the first place.
Please include that llist.h patch within the xen merge. It's just a single
hunk and won't cause any disruption.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-12-12 21:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-04 14:21 [PATCH 1/2] xen-blkback: implement safe iterator for the list of persistent grants Roger Pau Monne
2012-12-04 14:21 ` [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry Roger Pau Monne
2012-12-07 20:20 ` [Xen-devel] " Konrad Rzeszutek Wilk
2012-12-10 12:15 ` Roger Pau Monné
2012-12-10 15:15 ` Konrad Rzeszutek Wilk
2012-12-10 17:05 ` Roger Pau Monné
2012-12-12 21:01 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).