All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Abeni <pabeni@redhat.com>
To: Benjamin Coddington <bcodding@redhat.com>
Cc: netdev@vger.kernel.org,
	Trond Myklebust <trond.myklebust@primarydata.com>,
	Anna Schumaker <anna.schumaker@netapp.com>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	Jeff Layton <jlayton@poochiereds.net>,
	"David S. Miller" <davem@davemloft.net>,
	linux-nfs@vger.kernel.org, Jan Stancek <jstancek@redhat.com>
Subject: Re: [PATCH net] sunrpc: fix UDP memory accounting
Date: Wed, 31 Aug 2016 13:45:51 +0200	[thread overview]
Message-ID: <1472643951.4775.6.camel@redhat.com> (raw)
In-Reply-To: <F9224521-4FC7-4647-8BB8-C92A841EE23D@redhat.com>

On Tue, 2016-08-30 at 12:28 -0400, Benjamin Coddington wrote:
> On 25 Aug 2016, at 12:42, Paolo Abeni wrote:
> 
> > The commit f9b2ee714c5c ("SUNRPC: Move UDP receive data path
> > into a workqueue context"), as a side effect, moved the
> > skb_free_datagram() call outside the scope of the related socket
> > lock, but UDP sockets require such lock to be held for proper
> > memory accounting.
> > Fix it by replacing skb_free_datagram() with
> > skb_free_datagram_locked().
> >
> > Fixes: f9b2ee714c5c ("SUNRPC: Move UDP receive data path into a 
> > workqueue context")
> > Reported-and-tested-by: Jan Stancek <jstancek@redhat.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Thanks for finding this. A similar fix in 2009 for svcsock.c was done by 
> Eric Dumazet:
> 9d410c796067 ("net: fix sk_forward_alloc corruption")
> 
> skb_free_datagram_locked() is used for all xprt types in svcsock.c, 
> should we use
> it for the xs_local_transport as well in xprtsock.c?

AFAICS a similar fix is _not_ needed for xs_local_transport(), since it
uses AF_LOCAL sockets and such family does not support protocol's memory
accounting.

Paolo


WARNING: multiple messages have this Message-ID (diff)
From: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
To: Benjamin Coddington <bcodding-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Trond Myklebust
	<trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>,
	Anna Schumaker
	<anna.schumaker-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>,
	"J. Bruce Fields"
	<bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>,
	Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>,
	"David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Jan Stancek <jstancek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Subject: Re: [PATCH net] sunrpc: fix UDP memory accounting
Date: Wed, 31 Aug 2016 13:45:51 +0200	[thread overview]
Message-ID: <1472643951.4775.6.camel@redhat.com> (raw)
In-Reply-To: <F9224521-4FC7-4647-8BB8-C92A841EE23D-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

On Tue, 2016-08-30 at 12:28 -0400, Benjamin Coddington wrote:
> On 25 Aug 2016, at 12:42, Paolo Abeni wrote:
> 
> > The commit f9b2ee714c5c ("SUNRPC: Move UDP receive data path
> > into a workqueue context"), as a side effect, moved the
> > skb_free_datagram() call outside the scope of the related socket
> > lock, but UDP sockets require such lock to be held for proper
> > memory accounting.
> > Fix it by replacing skb_free_datagram() with
> > skb_free_datagram_locked().
> >
> > Fixes: f9b2ee714c5c ("SUNRPC: Move UDP receive data path into a 
> > workqueue context")
> > Reported-and-tested-by: Jan Stancek <jstancek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Thanks for finding this. A similar fix in 2009 for svcsock.c was done by 
> Eric Dumazet:
> 9d410c796067 ("net: fix sk_forward_alloc corruption")
> 
> skb_free_datagram_locked() is used for all xprt types in svcsock.c, 
> should we use
> it for the xs_local_transport as well in xprtsock.c?

AFAICS a similar fix is _not_ needed for xs_local_transport(), since it
uses AF_LOCAL sockets and such family does not support protocol's memory
accounting.

Paolo

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2016-08-31 11:45 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-25 16:42 [PATCH net] sunrpc: fix UDP memory accounting Paolo Abeni
2016-08-25 16:42 ` Paolo Abeni
2016-08-30 16:28 ` Benjamin Coddington
2016-08-30 16:28   ` Benjamin Coddington
2016-08-31 11:45   ` Paolo Abeni [this message]
2016-08-31 11:45     ` Paolo Abeni
2016-08-31 11:59     ` Benjamin Coddington
2016-08-31 11:59       ` Benjamin Coddington

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1472643951.4775.6.camel@redhat.com \
    --to=pabeni@redhat.com \
    --cc=anna.schumaker@netapp.com \
    --cc=bcodding@redhat.com \
    --cc=bfields@fieldses.org \
    --cc=davem@davemloft.net \
    --cc=jlayton@poochiereds.net \
    --cc=jstancek@redhat.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=trond.myklebust@primarydata.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.