On Mon, Oct 02, 2017 at 10:52:55PM +0900, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Mon 02-10-17 22:05:17, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Mon 02-10-17 20:33:52, Tetsuo Handa wrote:
> > > > > > I do not think that making oom notifier API more complex is the way to
> > > > > > go. Can we simply change the lock to try_lock?
> > > > >
> > > > > Using mutex_trylock(&vb->balloon_lock) alone is not sufficient. Inside the
> > > > > mutex, __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocation attempt is used
> > > > > which will fail to make progress due to oom_lock already held. Therefore,
> > > > > virtballoon_oom_notify() needs to guarantee that all allocation attempts use
> > > > > GFP_NOWAIT when called from virtballoon_oom_notify().
> > > >
> > > > Ohh, I missed your point and thought the dependency is indirect and some
> > > > other call path is allocating while holding the lock. But you seem to be
> > > > right and
> > > > leak_balloon
> > > > tell_host
> > > > virtqueue_add_outbuf
> > > > virtqueue_add
> > > >
> > > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should
> > > > try to allocate while we are in the OOM path. Michael, is there any way
> > > > to drop this?
> > >
> > > Michael already said
> > >
> > > That would be tricky to fix. I guess we'll need to drop the lock
> > > while allocating memory - not an easy fix.
> >
> > We are OOM, we cannot allocate _any_ memory! This is just broken.
> >
> > > and I think that it would be possible for virtio to locally offload
> > > virtballoon_oom_notify() using this patch's approach, if you don't like
> > > globally offloading at the OOM notifier API level.
> >
> > Even if the allocation is offloaded to a different context we are sill
> > OOM and we would have to block waiting for it which is just error prone.
>
> Like I comment below, I'm assuming that this deadlock should rarely
> happen from the beginning. Since GFP_KERNEL allocation is conditional,
> we might be able to avoid the allocation from virtballoon_oom_notify().
>
> Michael S. Tsirkin wrote:
> > > @@ -1005,17 +1033,21 @@ int unregister_oom_notifier(struct notifier_block *nb)
> > > */
> > > bool out_of_memory(struct oom_control *oc)
> > > {
> > > - unsigned long freed = 0;
> > > enum oom_constraint constraint = CONSTRAINT_NONE;
> > >
> > > if (oom_killer_disabled)
> > > return false;
> > >
> > > - if (!is_memcg_oom(oc)) {
> > > - blocking_notifier_call_chain(&oom_notify_list, 0, &freed);
> > > - if (freed > 0)
> > > + if (!is_memcg_oom(oc) && oom_notifier_th) {
> > > + oom_notifier_requested = true;
> > > + wake_up(&oom_notifier_request_wait);
> > > + wait_event_timeout(oom_notifier_response_wait,
> > > + !oom_notifier_requested, 5 * HZ);
> >
> > I guess this means what was earlier a deadlock will free up after 5
> > seconds,
>
> Yes.
>
> > by a 5 sec downtime is still a lot, isn't it?
>
> This timeout should unlikely expire. Please note that this offloading is
> intended for handling the worst scenario, that is, "out_of_memory() is called
> when somebody is already holding vb->balloon_lock lock" and
> "GFP_KERNEL allocation is attempted from virtballoon_oom_notify()".
>
> As far as I know, this lock is held when fill_balloon() or leak_balloon() is
> called. Majority of OOM events call out_of_memory() without holding this lock.
> Thus, "out_of_memory() is called when somebody is already holding vb->balloon_lock
> lock" should rarely happen from the beginning.
>
> If you can artificially trigger this deadlock (i.e. user triggerable OOM DoS),
> a patch for fixing this problem needs to be backported to older/distributor
> kernels...
>
> Yes, conditional GFP_KERNEL allocation attempt from virtqueue_add() might
> still cause this deadlock. But that depends on whether you can trigger this
> deadlock. As far as I know, there is no report. Thus, I think that avoiding
> theoretical deadlock using timeout will be sufficient.
So first of all IMHO GFP_KERNEL allocations do not happen in
virtqueue_add_outbuf at all. They only trigger through add_sgs.
IMHO this is an API bug, we should just drop the gfp parameter
from this API.
so the issue is balloon_page_enqueue only.
> >
> >
> > > + if (oom_notifier_freed) {
> > > + oom_notifier_freed = 0;
> > > /* Got some memory back in the last second. */
> > > return true;
> > > + }
> > > }
> > >
> > > /*
> > > --
> > > 1.8.3.1
> >
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
