seq_file API strangeness

seq_file API strangeness

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi!

While porting /proc/net/ip_conntrack over to seq_file, I stumbled across
the following problem:

The documentation says:

 *	seq_open() sets @file, associating it with a sequence described
 *	by @op.  @op->start() sets the iterator up and returns the first
 *	element of sequence. @op->stop() shuts it down.  @op->next()
 *	returns the next element of sequence.  @op->show() prints element
 *	into the buffer.  In case of error ->start() and ->next() return
 *	ERR_PTR(error).  In the end of sequence they return %NULL. ->show()
 *	returns 0 in case of success and negative number in case of error.

Now let's say I'm allocating some chunk of memory in ->start(), and
later on an error occurs.  Now I return ERR_PTR(something).  Later on, 
->stop() is called with that ERR_PTR(something) as parameter, and I try
to kfree() the chunk of memory that was allocated.  boom.  It's neither
NULL nor a valid pointer.

Also, I am wondering why the ->stop() function is called at all, when
->start() fails.  Initially, I was grabbing a lock, but only at the end
of ->start(), after all potential errors would already result in
returning ERR_PTR(something).  ->stop() however is then called
unconditionally, resulting in an unconditional unlock of my lock. boom.

Was this by intention?  I think it is unusual to call a  stop() function
even if start() didn't succeed.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: seq_file API strangeness

[viro]

On Fri, Nov 14, 2003 at 09:06:43PM +0100, Harald Welte wrote:
> Hi!
> 
> While porting /proc/net/ip_conntrack over to seq_file, I stumbled across
> the following problem:

> Now let's say I'm allocating some chunk of memory in ->start(), and
> later on an error occurs.  Now I return ERR_PTR(something).  Later on, 
> ->stop() is called with that ERR_PTR(something) as parameter, and I try
> to kfree() the chunk of memory that was allocated.  boom.  It's neither
> NULL nor a valid pointer.
> 
> Also, I am wondering why the ->stop() function is called at all, when
> ->start() fails.  Initially, I was grabbing a lock, but only at the end
> of ->start(), after all potential errors would already result in
> returning ERR_PTR(something).  ->stop() however is then called
> unconditionally, resulting in an unconditional unlock of my lock. boom.
> 
> Was this by intention?  I think it is unusual to call a  stop() function
> even if start() didn't succeed.

It is intentional.  In 99% of cases it ends up with cleaner methods and
in the rest you can trivially check the ->stop() argument.

Note that you *can* have failing ->next() do cleanup if you want to do
so.  In other words, instances that want such behaviour can get it easily.
And in common case you don't have to bother at all.

With "we call ->stop() only if..." you would still have to do the same amount
of work for hard cases *AND* get extra PITA for normal ones.  IOW, clear loss.

Re: seq_file API strangeness

[Tigran Aivazian]

Hi Al,

On the same subject, but a different issue:

In the ->open() method I allocate a seq->private like this:

  err = seq_open(file, sop);
  if (!err) {
	struct seq_file *m = file->private_data;

	m->private = kmalloc(sizeof(struct ctask), GFP_KERNEL);
        if (!m->private) {
                        kfree(file->private_data);
                        return -ENOMEM;
        }
  }

Now, freeing the structure that I did not allocate (file->private_data 
allocated in seq_open()) is not nice. But calling seq_release() from 
->open() method is not nice either (different arguments, namely 'inode'
and also m->buf is NULL at that point, although I believe kfree(NULL) is 
not illegal).

What do you think?

Kind regards
Tigran

Re: seq_file API strangeness

[viro]

On Fri, Nov 14, 2003 at 08:55:48PM +0000, Tigran Aivazian wrote:
> In the ->open() method I allocate a seq->private like this:
> 
>   err = seq_open(file, sop);
>   if (!err) {
> 	struct seq_file *m = file->private_data;
> 
> 	m->private = kmalloc(sizeof(struct ctask), GFP_KERNEL);
>         if (!m->private) {
>                         kfree(file->private_data);
>                         return -ENOMEM;
>         }
>   }
> 
> Now, freeing the structure that I did not allocate (file->private_data 
> allocated in seq_open()) is not nice. But calling seq_release() from 
> ->open() method is not nice either (different arguments, namely 'inode'

I beg your pardon?  What different arguments?

->open() gets struct inode * and struct file *
->release() gets exactly the same.
seq_release() is what you use as ->release()

What's the problem?

> and also m->buf is NULL at that point, although I believe kfree(NULL) is 
> not illegal).

Of course it is not illegal.  Moreover, if you just do open() immediately
followed by close(), you won't get non-NULL ->buf at all.  It's a perfectly
normal situation and seq_release() can handle it - no problems with that.

> What do you think?

	if (!m->private) {
		seq_release(inode, file);
		return -ENOMEM;
	}

Same as e.g. fs/proc/base.c does in similar situation (see mounts_open()).

Re: seq_file API strangeness

[Tigran Aivazian]

Hi Al,

Yes, you are right, thank you. I don't know why I thought open/release 
took different arguments. Yes, calling seq_release(inode,file) is the 
right way, I will change my code.

Kind regards
Tigran

On Fri, 14 Nov 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:

> On Fri, Nov 14, 2003 at 08:55:48PM +0000, Tigran Aivazian wrote:
> > In the ->open() method I allocate a seq->private like this:
> > 
> >   err = seq_open(file, sop);
> >   if (!err) {
> > 	struct seq_file *m = file->private_data;
> > 
> > 	m->private = kmalloc(sizeof(struct ctask), GFP_KERNEL);
> >         if (!m->private) {
> >                         kfree(file->private_data);
> >                         return -ENOMEM;
> >         }
> >   }
> > 
> > Now, freeing the structure that I did not allocate (file->private_data 
> > allocated in seq_open()) is not nice. But calling seq_release() from 
> > ->open() method is not nice either (different arguments, namely 'inode'
> 
> I beg your pardon?  What different arguments?
> 
> ->open() gets struct inode * and struct file *
> ->release() gets exactly the same.
> seq_release() is what you use as ->release()
> 
> What's the problem?
> 
> > and also m->buf is NULL at that point, although I believe kfree(NULL) is 
> > not illegal).
> 
> Of course it is not illegal.  Moreover, if you just do open() immediately
> followed by close(), you won't get non-NULL ->buf at all.  It's a perfectly
> normal situation and seq_release() can handle it - no problems with that.
> 
> > What do you think?
> 
> 	if (!m->private) {
> 		seq_release(inode, file);
> 		return -ENOMEM;
> 	}
> 
> Same as e.g. fs/proc/base.c does in similar situation (see mounts_open()).
> 

Re: seq_file API strangeness

[Tigran Aivazian]

Ah, I know why I thought they took different arguments! Because I was 
creating two files --- binary and ascii versions and they shared the same 
"open_common" routine which didn't need an 'inode' so I was only passing 
'file' and appropriate 'seq_operations' pointer to it :)

On Sat, 15 Nov 2003, Tigran Aivazian wrote:

> Hi Al,
> 
> Yes, you are right, thank you. I don't know why I thought open/release 
> took different arguments. Yes, calling seq_release(inode,file) is the 
> right way, I will change my code.
> 
> Kind regards
> Tigran
> 
> On Fri, 14 Nov 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:
> 
> > On Fri, Nov 14, 2003 at 08:55:48PM +0000, Tigran Aivazian wrote:
> > > In the ->open() method I allocate a seq->private like this:
> > > 
> > >   err = seq_open(file, sop);
> > >   if (!err) {
> > > 	struct seq_file *m = file->private_data;
> > > 
> > > 	m->private = kmalloc(sizeof(struct ctask), GFP_KERNEL);
> > >         if (!m->private) {
> > >                         kfree(file->private_data);
> > >                         return -ENOMEM;
> > >         }
> > >   }
> > > 
> > > Now, freeing the structure that I did not allocate (file->private_data 
> > > allocated in seq_open()) is not nice. But calling seq_release() from 
> > > ->open() method is not nice either (different arguments, namely 'inode'
> > 
> > I beg your pardon?  What different arguments?
> > 
> > ->open() gets struct inode * and struct file *
> > ->release() gets exactly the same.
> > seq_release() is what you use as ->release()
> > 
> > What's the problem?
> > 
> > > and also m->buf is NULL at that point, although I believe kfree(NULL) is 
> > > not illegal).
> > 
> > Of course it is not illegal.  Moreover, if you just do open() immediately
> > followed by close(), you won't get non-NULL ->buf at all.  It's a perfectly
> > normal situation and seq_release() can handle it - no problems with that.
> > 
> > > What do you think?
> > 
> > 	if (!m->private) {
> > 		seq_release(inode, file);
> > 		return -ENOMEM;
> > 	}
> > 
> > Same as e.g. fs/proc/base.c does in similar situation (see mounts_open()).
> > 
> 
>