Hi,
the following patch corrects a bug where rmap would continue
trying to swap out a page even after it failed on one pte,
which could result in leaked pte chains and a bug when exiting
applications which use mlock().
The bug was tracked down by Christian Ehrhardt, the reason
it wasn't found earlier was a subtlety in the code, so I've
taken the liberty of changing Christian's patch into something
more explicit, we shouldn't let this one happen again ;)
please apply,
Rik
--
Bravely reimplemented by the knights who say "NIH".
http://www.surriel.com/ http://distro.conectiva.com/
===== mm/rmap.c 1.7 vs edited =====
--- 1.7/mm/rmap.c Wed Jul 31 06:58:53 2002
+++ edited/mm/rmap.c Mon Aug 12 11:22:05 2002
@@ -328,7 +328,7 @@
case SWAP_SUCCESS:
/* Free the pte_chain struct. */
pte_chain_free(pc, prev_pc, page);
- break;
+ continue;
case SWAP_AGAIN:
/* Skip this pte, remembering status. */
prev_pc = pc;
@@ -336,12 +336,13 @@
continue;
case SWAP_FAIL:
ret = SWAP_FAIL;
- break;
+ goto give_up;
case SWAP_ERROR:
ret = SWAP_ERROR;
- break;
+ goto give_up;
}
}
+give_up:
/* Check whether we can convert to direct pte pointer */
pc = page->pte.chain;
if (pc && !pc->next) {
On Monday 12 August 2002 16:58, Rik van Riel wrote:
> case SWAP_FAIL:
> ret = SWAP_FAIL;
> - break;
> + goto give_up;
Yes, I looked at that many times while reading the break as a 'break
from loop' every time. Using the same keyword to mean 'stop looping'
and 'endcase' was, by any measure, a stupid idea.
--
Daniel
On Mon, 19 Aug 2002, Daniel Phillips wrote:
> On Monday 12 August 2002 16:58, Rik van Riel wrote:
> > case SWAP_FAIL:
> > ret = SWAP_FAIL;
> > - break;
> > + goto give_up;
>
> Yes, I looked at that many times while reading the break as a 'break
> from loop' every time. Using the same keyword to mean 'stop looping'
> and 'endcase' was, by any measure, a stupid idea.
What's even more curious is that 'continue' has the exact
same effect on 'switch' ...
regards,
Rik
--
Bravely reimplemented by the knights who say "NIH".
http://www.surriel.com/ http://distro.conectiva.com/
On Monday 19 August 2002 22:15, Rik van Riel wrote:
> On Mon, 19 Aug 2002, Daniel Phillips wrote:
> > On Monday 12 August 2002 16:58, Rik van Riel wrote:
> > > case SWAP_FAIL:
> > > ret = SWAP_FAIL;
> > > - break;
> > > + goto give_up;
> >
> > Yes, I looked at that many times while reading the break as a 'break
> > from loop' every time. Using the same keyword to mean 'stop looping'
> > and 'endcase' was, by any measure, a stupid idea.
>
> What's even more curious is that 'continue' has the exact
> same effect on 'switch' ...
Come to think of it, what you want there is:
case SWAP_FAIL:
return SWAP_FAIL;
--
Daniel
On Mon, 19 Aug 2002, Daniel Phillips wrote:
> On Monday 19 August 2002 22:15, Rik van Riel wrote:
> > On Mon, 19 Aug 2002, Daniel Phillips wrote:
> > > On Monday 12 August 2002 16:58, Rik van Riel wrote:
> > > > case SWAP_FAIL:
> > > > ret = SWAP_FAIL;
> > > > - break;
> > > > + goto give_up;
> > >
> > > Yes, I looked at that many times while reading the break as a 'break
> > > from loop' every time. Using the same keyword to mean 'stop looping'
> > > and 'endcase' was, by any measure, a stupid idea.
> >
> > What's even more curious is that 'continue' has the exact
> > same effect on 'switch' ...
>
> Come to think of it, what you want there is:
>
> case SWAP_FAIL:
> return SWAP_FAIL;
We still want to try to convert the thing to a direct pointer,
in case only the last task was using mlock().
regards,
Rik
--
Bravely reimplemented by the knights who say "NIH".
http://www.surriel.com/ http://distro.conectiva.com/