linux-kernel - Re: Re: [Xen-devel] [PATCH v10 2/4] xen/blkback: Squeeze page pools if a memory pressure is detected

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191216161549.26976-1-sjpark@amazon.com>
Date:   Mon, 16 Dec 2019 17:15:49 +0100
From:   SeongJae Park <sjpark@...zon.com>
To:     <jgross@...e.com>, <axboe@...nel.dk>, <konrad.wilk@...cle.com>,
        <roger.pau@...rix.com>
CC:     <pdurrant@...zon.com>, <linux-kernel@...r.kernel.org>,
        <linux-block@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
        <sj38.park@...il.com>, <sjpark@...zon.com>
Subject: Re: Re: [Xen-devel] [PATCH v10 2/4] xen/blkback: Squeeze page pools if a memory pressure is detected

On Mon, 16 Dec 2019 15:37:20 +0100 SeongJae Park <sjpark@...zon.com> wrote:

> On Mon, 16 Dec 2019 13:45:25 +0100 SeongJae Park <sjpark@...zon.com> wrote:
> 
> > From: SeongJae Park <sjpark@...zon.de>
> > 
[...]
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -824,6 +824,24 @@ static void frontend_changed(struct xenbus_device *dev,
> >  }
> >  
> >  
> > +/* Once a memory pressure is detected, squeeze free page pools for a while. */
> > +static unsigned int buffer_squeeze_duration_ms = 10;
> > +module_param_named(buffer_squeeze_duration_ms,
> > +		buffer_squeeze_duration_ms, int, 0644);
> > +MODULE_PARM_DESC(buffer_squeeze_duration_ms,
> > +"Duration in ms to squeeze pages buffer when a memory pressure is detected");
> > +
> > +/*
> > + * Callback received when the memory pressure is detected.
> > + */
> > +static void reclaim_memory(struct xenbus_device *dev)
> > +{
> > +	struct backend_info *be = dev_get_drvdata(&dev->dev);
> > +
> > +	be->blkif->buffer_squeeze_end = jiffies +
> > +		msecs_to_jiffies(buffer_squeeze_duration_ms);
> 
> This callback might race with 'xen_blkbk_probe()'.  The race could result in
> __NULL dereferencing__, as 'xen_blkbk_probe()' sets '->blkif' after it links
> 'be' to the 'dev'.  Please _don't merge_ this patch now!
> 
> I will do more test and share results.  Meanwhile, if you have any opinion,
> please let me know.

Not only '->blkif', but 'be' itself also coule be a NULL.  As similar
concurrency issues could be in other drivers in their way, I suggest to change
the reclaim callback ('->reclaim_memory') to be called for each driver instead
of each device.  Then, each driver could be able to deal with its concurrency
issues by itself.

For blkback, we could reuse the global variable based approach, as similar to
the v7[1] of this patchset.  As the callback is called for each driver instead
of each device now, the duplicated set of the timeout will not happen.


Thanks,
SeongJae Park

[1] https://lore.kernel.org/xen-devel/20191211181016.14366-1-sjpark@amazon.de/

> 
> 
> Thanks,
> SeongJae Park
> 
> > +}
> > +
> >  /* ** Connection ** */
> >  
> >  
> > @@ -1115,7 +1133,8 @@ static struct xenbus_driver xen_blkbk_driver = {
> >  	.ids  = xen_blkbk_ids,
> >  	.probe = xen_blkbk_probe,
> >  	.remove = xen_blkbk_remove,
> > -	.otherend_changed = frontend_changed
> > +	.otherend_changed = frontend_changed,
> > +	.reclaim_memory = reclaim_memory,
> >  };
> >  
> >  int xen_blkif_xenbus_init(void)
> > -- 
> > 2.17.1
> > 
>