Re: [Qemu-devel] [PATCH] tap-linux: report an error for invalid sndbuf

[Michael S. Tsirkin]

On Mon, Apr 28, 2014 at 12:06:49PM +0800, Amos Kong wrote:
> On Sun, Apr 27, 2014 at 08:16:58PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 28, 2014 at 01:03:04AM +0800, Amos Kong wrote:
> > > On Sun, Apr 27, 2014 at 05:20:28PM +0300, Michael S. Tsirkin wrote:
> > > > On Sun, Apr 27, 2014 at 10:09:10PM +0800, Amos Kong wrote:
> > > > > We use default sndbuf (INT_MAX) if user assigns an invalid sndbuf.
> > > > > This patch just added an error note.
> > > > > 
> > > > > Signed-off-by: Amos Kong <address@hidden>
> > > > > ---
> > > > >  net/tap-linux.c | 13 ++++++++++---
> > > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/net/tap-linux.c b/net/tap-linux.c
> > > > > index 812bf2d..fc0cc0d 100644
> > > > > --- a/net/tap-linux.c
> > > > > +++ b/net/tap-linux.c
> > > > > @@ -130,9 +130,16 @@ int tap_set_sndbuf(int fd, const 
> > > > > NetdevTapOptions *tap)
> > > > >  {
> > > > >      int sndbuf;
> > > > >  
> > > > > -    sndbuf = !tap->has_sndbuf       ? TAP_DEFAULT_SNDBUF :
> > > > > -             tap->sndbuf > INT_MAX  ? INT_MAX :
> > > > > -             tap->sndbuf;
> > > > > +    if (!tap->has_sndbuf) {
> > > > > +        sndbuf = TAP_DEFAULT_SNDBUF;
> > > > > +    } else if (tap->sndbuf > INT_MAX) {
> > > > > +        sndbuf = TAP_DEFAULT_SNDBUF;
> > > > > +        error_report("given sndbuf isn't an integer, "
> > > > > +                     "or it's larger than INT_MAX(%d). "
> > > > > +                     "use default sndbuf(%d)", INT_MAX, INT_MAX);
> > > > 
> > > > I think that converting a huge value to INT_MAX is
> > > > actually more reasonable.
> > > 
> > > Yes, this is the current behavir.
> > > 
> > > 
> > > Current TAP_DEFAULT_SNDBUF is zero ....
> > > 
> > > > For example, comment in json schema says:
> > > > # @sndbuf: #optional send buffer limit. Understands [TGMKkb] suffixes.
> > > > If someone sets it to 1T why not make it work?
> > > 
> > > > Failing if it's not an integer sounds like a good feature.
> > > 
> > > 
> > > I posted another patch to fix qapi/opts-visitor.c
> > > Then too huge value will be catched in that place, and report an error.
> > > 
> > > I tested with sndbuf=8388607T, it can pass fixed qapi checking, but it
> > > will be converted to INT_MAX in tap_set_sndbuf().
> > > 
> > > 
> > > We can extend the buf size by changing the type of 'sndbuf' from 'int'
> > > to 'int64_t'. the type of skbbuf in kernel also need to be updated.
>  
> Hi Michael,
> 
> > The assumption always was that INT_MAX has same effect as infinity for tap.
> > Do you really expect INT_MAX size to be exhausted by just one tap
> > device?
> >
> > If we ever hit a configuration where that's not the case, I would say
> > as the first step, let's make INT_MAX a practical infinity in kernel.
> > I'd like to see a test case like this first though.
> 
> 
> This patch [1] only added a hint if huge value that is larger than
> INT_MAX is changed to INT_MAX. We can't change it silently.

Why can't we? Seems safe to me.

> The patch [2] fixed a qapi error, sndbuf accepts 'size' type (that is 
> int64_t).
> 
> [1] [PATCH] tap-linux: report an error for invalid
> [2] [PATCH] qapi: treat all negative return of strtosz_suffix() as error

2 seems to make sense, will review.

> 
> To adjust the max limitation is another issue, I think we can only
> adjust the limitation when we have real usecase (I don't have).
> 
> Thanks, Amos
> 
> > > I will have a try.
> > >  
> > > > > +    } else {
> > > > > +        sndbuf = tap->sndbuf;
> > > > > +    }
> > > > >  
> > > > >      if (!sndbuf) {
> > > > >          sndbuf = INT_MAX;
> > > 
> > > ... zero sndbuf will be converted to INT_MAX here.