[linux-cifs-client] [PATCH] Cannot allocate memory

[Jeff Layton]

On Wed, 11 Mar 2009 00:43:28 +0300
Pavel Shilovsky <piastry at etersoft.ru> wrote:

> On Tuesday 10 March 2009 22:01:28 you wrote:
> > On Tue, 10 Mar 2009 18:31:36 +0300
> >
> > Pavel Shilovsky <piastry at etersoft.ru> wrote:
> > > Hello!
> > >
> > > This patch resolves problem "Cannot allocate memory" during loading
> > > module when we do unload/load quickly. During unloading the
> > > cifs_demultiplex_thread doesn't have enough time for clearing cache and
> > > kmem_cache_destroy fails. Then during loading module it fails with
> > > kmem_cache_create: duplicate cache cifs_request.
> > >
> > > This script can show this bug:
> > >
> 
> > > rmmod cifs
> > >
> > > for i in `seq 1 20`;
> > > do {
> > >         echo "trying to mount " $i
> > >         insmod ./cifs.ko
> > >         cifsmount [имя шары] [путь] -onoperm
> Sorry, I forgot to change this line to:
> 	      cifsmount [share name] [path] -onoperm
> > >         umount [путь]
> > >         rmmod cifs
> > > };
> > > done
> > >
> > >
> > >
> > > And it fails on 2nd iteration.
> > >
> > > My patch fixes it.
> > >
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index 1ae6314..18ba45a 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -176,6 +176,7 @@ struct TCP_Server_Info {
> > >         struct mac_key mac_signing_key;
> > >         char ntlmv2_hash[16];
> > >         unsigned long lstrp; /* when we got last response from this
> > > server */
> > > +       int running;
> > >  };
> > >
> > >  /*
> > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > > index f254235..39daaf2 100644
> > > --- a/fs/cifs/connect.c
> > > +++ b/fs/cifs/connect.c
> > > @@ -347,6 +347,7 @@ cifs_demultiplex_thread(struct TCP_Server_Info
> > > *server) bool isMultiRsp;
> > >         int reconnect;
> > >
> > > +       server->running = 1;
> > >         current->flags |= PF_MEMALLOC;
> > >         cFYI(1, ("Demultiplex PID: %d", task_pid_nr(current)));
> > >
> > > @@ -758,6 +759,8 @@ multi_t2_fnd:
> > >
> > >         kfree(server->hostname);
> > >         task_to_wake = xchg(&server->tsk, NULL);
> > > +       server->running = 0;
> > > +       msleep(20);
> > >         kfree(server);
> > >
> > >         length = atomic_dec_return(&tcpSesAllocCount);
> > > @@ -1408,6 +1411,9 @@ cifs_put_tcp_session(struct TCP_Server_Info
> > > *server) task = xchg(&server->tsk, NULL);
> > >         if (task)
> > >                 force_sig(SIGKILL, task);
> > > +
> > > +       while(server->running == 1)
> > > +               msleep(10);
> > >  }
> > >
> > >  static struct cifsSesInfo *
> >
> > On what kernel version are you reproducing this? Does it have this patch?
> >
> > commit 0468a2cf914e79442b8309ce62e3f861599d8cab
> > Author: Jeff Layton <jlayton at redhat.com>
> > Date:   Mon Dec 1 07:09:35 2008 -0500
> >
> >     cifs: take module reference when starting cifsd
> >
> >     cifsd can outlive the last cifs mount. We need to hold a module
> >     reference until it exits to prevent someone from unplugging
> >     the module until we're ready.
> >
> >     Signed-off-by: Jeff Layton <jlayton at redhat.com>
> >     Signed-off-by: Steve French <sfrench at us.ibm.com>
> I have 2.6.27 version of kernel and it doesn't contain this code. I added it 
> and tested.

(re-cc'ing linux-cifs-client)

Ok, it's much more helpful to post patches that are based on current
upstream code. For cifs, the ideal is to base (and test) your patches
on Steve F's tree.

> Result: Umounting share finish and when I try to do "rmmod cifs" it says that 
> module is in use. I have to wait some time to do it successfully.
> 

That's what I'd expect. The module is still in use at that time,
regardless of whether any mounts are still active.

> My patch provides another behaviour: when umounting share finishes I can remove 
> module at once.  
> 

Your patch simply adds an arbitrary delay in unmounting. There's no
guarantee that 10ms is enough time for all of the shutdown processing
in cifsd to occur. It also adds a 20ms delay for the thread itself to
come down. What's that all about?

Finally, it's also racy:

> >         if (task)
> >                 force_sig(SIGKILL, task);
> > +
> > +       while(server->running == 1)
> > +               msleep(10);

You have no guarantee that "server" is still a valid pointer when you
go to dereference it. In most cases it probably will be, but hard to
hit races are *worse* since they're harder to track down.

We have to *know* that this pointer will be valid here before you do
something like this. Rather than adding arbitrary delays like this, a
better scheme would use a completion variable to indicate when the
thread is down.

Finally, I'm not convinced of the need for this at all. Why should we
guarantee that the module is able to be unplugged the instant that the
last mount vanishes? I'd much rather my umounts return quickly rather
than have them wait for the thread to come down.

-- 
Jeff Layton <jlayton at redhat.com>