Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry
roger.pau at citrix
Dec 4, 2012, 6:21 AM
Post #1 of 6 (413 views)
Permalink
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)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry [ In reply to ]
konrad.wilk at oracle
Dec 7, 2012, 12:20 PM
Post #2 of 6 (391 views)
Permalink
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
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry [ In reply to ]
roger.pau at citrix
Dec 10, 2012, 4:15 AM
Post #3 of 6 (391 views)
Permalink
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?


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry [ In reply to ]
konrad.wilk at oracle
Dec 10, 2012, 7:15 AM
Post #4 of 6 (390 views)
Permalink
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.
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry [ In reply to ]
roger.pau at citrix
Dec 10, 2012, 9:05 AM
Post #5 of 6 (390 views)
Permalink
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.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 2/2] xen-blkfront: implement safe version of llist_for_each_entry [ In reply to ]
akpm at linux-foundation
Dec 12, 2012, 1:01 PM
Post #6 of 6 (392 views)
Permalink
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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal