All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: Change netdev_fix_features messages loglevel
@ 2011-05-16 13:17 Michał Mirosław
  2011-05-16 19:14 ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: Michał Mirosław @ 2011-05-16 13:17 UTC (permalink / raw)
  To: netdev; +Cc: Michael S. Tsirkin, Herbert Xu, Ben Hutchings

Those reduced to DEBUG can possibly be triggered by unprivileged processes
and are nothing exceptional. Illegal checksum combinations can only be
caused by driver bug, so promote those messages to WARN.

Since GSO without SG will now only cause DEBUG message from
netdev_fix_features(), remove the workaround from register_netdevice().

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---

This is for net tree, not net-next.

 net/core/dev.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 9200944..b624fe4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5186,27 +5186,27 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 	/* Fix illegal checksum combinations */
 	if ((features & NETIF_F_HW_CSUM) &&
 	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-		netdev_info(dev, "mixed HW and IP checksum settings.\n");
+		netdev_warn(dev, "mixed HW and IP checksum settings.\n");
 		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
 	}
 
 	if ((features & NETIF_F_NO_CSUM) &&
 	    (features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-		netdev_info(dev, "mixed no checksumming and other settings.\n");
+		netdev_warn(dev, "mixed no checksumming and other settings.\n");
 		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
 	}
 
 	/* Fix illegal SG+CSUM combinations. */
 	if ((features & NETIF_F_SG) &&
 	    !(features & NETIF_F_ALL_CSUM)) {
-		netdev_info(dev,
-			    "Dropping NETIF_F_SG since no checksum feature.\n");
+		netdev_dbg(dev,
+			"Dropping NETIF_F_SG since no checksum feature.\n");
 		features &= ~NETIF_F_SG;
 	}
 
 	/* TSO requires that SG is present as well. */
 	if ((features & NETIF_F_ALL_TSO) && !(features & NETIF_F_SG)) {
-		netdev_info(dev, "Dropping TSO features since no SG feature.\n");
+		netdev_dbg(dev, "Dropping TSO features since no SG feature.\n");
 		features &= ~NETIF_F_ALL_TSO;
 	}
 
@@ -5216,7 +5216,7 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 
 	/* Software GSO depends on SG. */
 	if ((features & NETIF_F_GSO) && !(features & NETIF_F_SG)) {
-		netdev_info(dev, "Dropping NETIF_F_GSO since no SG feature.\n");
+		netdev_dbg(dev, "Dropping NETIF_F_GSO since no SG feature.\n");
 		features &= ~NETIF_F_GSO;
 	}
 
@@ -5226,13 +5226,13 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 		if (!((features & NETIF_F_GEN_CSUM) ||
 		    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
 			    == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-			netdev_info(dev,
+			netdev_dbg(dev,
 				"Dropping NETIF_F_UFO since no checksum offload features.\n");
 			features &= ~NETIF_F_UFO;
 		}
 
 		if (!(features & NETIF_F_SG)) {
-			netdev_info(dev,
+			netdev_dbg(dev,
 				"Dropping NETIF_F_UFO since no NETIF_F_SG feature.\n");
 			features &= ~NETIF_F_UFO;
 		}
@@ -5414,12 +5414,6 @@ int register_netdevice(struct net_device *dev)
 	dev->features |= NETIF_F_SOFT_FEATURES;
 	dev->wanted_features = dev->features & dev->hw_features;
 
-	/* Avoid warning from netdev_fix_features() for GSO without SG */
-	if (!(dev->wanted_features & NETIF_F_SG)) {
-		dev->wanted_features &= ~NETIF_F_GSO;
-		dev->features &= ~NETIF_F_GSO;
-	}
-
 	/* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
 	 * vlan_dev_init() will do the dev->features check, so these features
 	 * are enabled only if supported by underlying device.


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] net: Change netdev_fix_features messages loglevel
  2011-05-16 13:17 [PATCH] net: Change netdev_fix_features messages loglevel Michał Mirosław
@ 2011-05-16 19:14 ` David Miller
  2011-05-16 20:37   ` Michael S. Tsirkin
  2011-05-16 23:48   ` Herbert Xu
  0 siblings, 2 replies; 6+ messages in thread
From: David Miller @ 2011-05-16 19:14 UTC (permalink / raw)
  To: mirq-linux; +Cc: netdev, mst, herbert, bhutchings

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Mon, 16 May 2011 15:17:57 +0200 (CEST)

> Those reduced to DEBUG can possibly be triggered by unprivileged processes
> and are nothing exceptional. Illegal checksum combinations can only be
> caused by driver bug, so promote those messages to WARN.
> 
> Since GSO without SG will now only cause DEBUG message from
> netdev_fix_features(), remove the workaround from register_netdevice().
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied, thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] net: Change netdev_fix_features messages loglevel
  2011-05-16 19:14 ` David Miller
@ 2011-05-16 20:37   ` Michael S. Tsirkin
  2011-05-17 19:45     ` David Miller
  2011-05-16 23:48   ` Herbert Xu
  1 sibling, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2011-05-16 20:37 UTC (permalink / raw)
  To: David Miller; +Cc: mirq-linux, netdev, herbert, bhutchings

On Mon, May 16, 2011 at 03:14:34PM -0400, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Mon, 16 May 2011 15:17:57 +0200 (CEST)
> 
> > Those reduced to DEBUG can possibly be triggered by unprivileged processes
> > and are nothing exceptional. Illegal checksum combinations can only be
> > caused by driver bug, so promote those messages to WARN.
> > 
> > Since GSO without SG will now only cause DEBUG message from
> > netdev_fix_features(), remove the workaround from register_netdevice().
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> Applied, thanks.

Cool, how about we make 'Features changed' debug as well?
This way userspace can't fill up the log just by tweaking tun features
with an ioctl.

Untested, but you get the message.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/net/core/dev.c b/net/core/dev.c
index 3ed09f8..538a1fe 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5270,7 +5270,7 @@ int __netdev_update_features(struct net_device *dev)
 	if (dev->features == features)
 		return 0;
 
-	netdev_info(dev, "Features changed: 0x%08x -> 0x%08x\n",
+	netdev_dbg(dev, "Features changed: 0x%08x -> 0x%08x\n",
 		dev->features, features);
 
 	if (dev->netdev_ops->ndo_set_features)
-- 
MST

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] net: Change netdev_fix_features messages loglevel
  2011-05-16 19:14 ` David Miller
  2011-05-16 20:37   ` Michael S. Tsirkin
@ 2011-05-16 23:48   ` Herbert Xu
  2011-05-17  8:27     ` Michał Mirosław
  1 sibling, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2011-05-16 23:48 UTC (permalink / raw)
  To: David Miller; +Cc: mirq-linux, netdev, mst, bhutchings

On Mon, May 16, 2011 at 03:14:34PM -0400, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Mon, 16 May 2011 15:17:57 +0200 (CEST)
> 
> > Those reduced to DEBUG can possibly be triggered by unprivileged processes
> > and are nothing exceptional. Illegal checksum combinations can only be
> > caused by driver bug, so promote those messages to WARN.
> > 
> > Since GSO without SG will now only cause DEBUG message from
> > netdev_fix_features(), remove the workaround from register_netdevice().
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> 
> Applied, thanks.

I think this patch is not a good idea.  The only problem here
is the tun driver which should just fix up the feature bits
silently (we need to do that anyway as right now it doesn't
verify the feature bits at all, those warnings are only trigger
on registering the tun device).

With this patch future buggy drivers will instead trigger hard-
to-debug warnings further down the stack in the TSO code path,
like they have before (or silent packet drops if those warnings
aren't there anymore).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] net: Change netdev_fix_features messages loglevel
  2011-05-16 23:48   ` Herbert Xu
@ 2011-05-17  8:27     ` Michał Mirosław
  0 siblings, 0 replies; 6+ messages in thread
From: Michał Mirosław @ 2011-05-17  8:27 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David Miller, netdev, mst, bhutchings

On Tue, May 17, 2011 at 09:48:16AM +1000, Herbert Xu wrote:
> On Mon, May 16, 2011 at 03:14:34PM -0400, David Miller wrote:
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Date: Mon, 16 May 2011 15:17:57 +0200 (CEST)
> > 
> > > Those reduced to DEBUG can possibly be triggered by unprivileged processes
> > > and are nothing exceptional. Illegal checksum combinations can only be
> > > caused by driver bug, so promote those messages to WARN.
> > > 
> > > Since GSO without SG will now only cause DEBUG message from
> > > netdev_fix_features(), remove the workaround from register_netdevice().
> > > 
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Applied, thanks.
> I think this patch is not a good idea.  The only problem here
> is the tun driver which should just fix up the feature bits
> silently (we need to do that anyway as right now it doesn't
> verify the feature bits at all, those warnings are only trigger
> on registering the tun device).
> 
> With this patch future buggy drivers will instead trigger hard-
> to-debug warnings further down the stack in the TSO code path,
> like they have before (or silent packet drops if those warnings
> aren't there anymore).

If core TSO code really depends on checksumming features being turned on
then this dependency should be checked in netdev_fix_features. There
will be no possibility for a driver to cause bugs then.

Network core is making up for devices which can't checksum just before
calling ndo_hard_start_xmit. It's a possibility that packet to be TSOed
will be unnecessarily checksummed in dev_hard_start_xmit() if HW checksum
is off while TSO is on (I deduce this based on the code, can't test as
TSO in my test-box is busted).

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] net: Change netdev_fix_features messages loglevel
  2011-05-16 20:37   ` Michael S. Tsirkin
@ 2011-05-17 19:45     ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2011-05-17 19:45 UTC (permalink / raw)
  To: mst; +Cc: mirq-linux, netdev, herbert, bhutchings

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 16 May 2011 23:37:39 +0300

> On Mon, May 16, 2011 at 03:14:34PM -0400, David Miller wrote:
>> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> Date: Mon, 16 May 2011 15:17:57 +0200 (CEST)
>> 
>> > Those reduced to DEBUG can possibly be triggered by unprivileged processes
>> > and are nothing exceptional. Illegal checksum combinations can only be
>> > caused by driver bug, so promote those messages to WARN.
>> > 
>> > Since GSO without SG will now only cause DEBUG message from
>> > netdev_fix_features(), remove the workaround from register_netdevice().
>> > 
>> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> 
>> Applied, thanks.
> 
> Cool, how about we make 'Features changed' debug as well?
> This way userspace can't fill up the log just by tweaking tun features
> with an ioctl.
> 
> Untested, but you get the message.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Agreed and applied.

I agree with Herbert's long term assertion that we should handle this
differently, somehow, to avoid these issues.

But as long as userspace can trigger these events, we have to do things
like decrease the logging level of these messages.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-17 19:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-16 13:17 [PATCH] net: Change netdev_fix_features messages loglevel Michał Mirosław
2011-05-16 19:14 ` David Miller
2011-05-16 20:37   ` Michael S. Tsirkin
2011-05-17 19:45     ` David Miller
2011-05-16 23:48   ` Herbert Xu
2011-05-17  8:27     ` Michał Mirosław

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.