* [PATCH net 1/2] net: Use device model to get driver name in skb_gso_segment() @ 2012-01-16 22:38 Ben Hutchings 2012-01-16 22:41 ` Herbert Xu 2012-01-16 22:43 ` [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation Ben Hutchings 0 siblings, 2 replies; 7+ messages in thread From: Ben Hutchings @ 2012-01-16 22:38 UTC (permalink / raw) To: David Miller; +Cc: Herbert Xu, netdev ethtool operations generally require the caller to hold RTNL and are not safe to call in atomic context. The device model provides this information for most devices; we'll only lose it for some old ISA drivers. Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> --- net/core/dev.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index f494675..7e6b7dc 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1962,13 +1962,13 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { struct net_device *dev = skb->dev; - struct ethtool_drvinfo info = {}; + const char *driver = ""; - if (dev && dev->ethtool_ops && dev->ethtool_ops->get_drvinfo) - dev->ethtool_ops->get_drvinfo(dev, &info); + if (dev && dev->dev.parent) + driver = dev_driver_string(dev->dev.parent); WARN(1, "%s: caps=(%pNF, %pNF) len=%d data_len=%d ip_summed=%d\n", - info.driver, dev ? &dev->features : NULL, + driver, dev ? &dev->features : NULL, skb->sk ? &skb->sk->sk_route_caps : NULL, skb->len, skb->data_len, skb->ip_summed); -- 1.7.7.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] net: Use device model to get driver name in skb_gso_segment() 2012-01-16 22:38 [PATCH net 1/2] net: Use device model to get driver name in skb_gso_segment() Ben Hutchings @ 2012-01-16 22:41 ` Herbert Xu 2012-01-17 15:32 ` David Miller 2012-01-16 22:43 ` [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation Ben Hutchings 1 sibling, 1 reply; 7+ messages in thread From: Herbert Xu @ 2012-01-16 22:41 UTC (permalink / raw) To: Ben Hutchings; +Cc: David Miller, netdev On Mon, Jan 16, 2012 at 10:38:59PM +0000, Ben Hutchings wrote: > ethtool operations generally require the caller to hold RTNL and are > not safe to call in atomic context. The device model provides this > information for most devices; we'll only lose it for some old ISA > drivers. > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Thanks, -- 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] 7+ messages in thread
* Re: [PATCH net 1/2] net: Use device model to get driver name in skb_gso_segment() 2012-01-16 22:41 ` Herbert Xu @ 2012-01-17 15:32 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2012-01-17 15:32 UTC (permalink / raw) To: herbert; +Cc: bhutchings, netdev From: Herbert Xu <herbert@gondor.hengli.com.au> Date: Tue, 17 Jan 2012 09:41:38 +1100 > On Mon, Jan 16, 2012 at 10:38:59PM +0000, Ben Hutchings wrote: >> ethtool operations generally require the caller to hold RTNL and are >> not safe to call in atomic context. The device model provides this >> information for most devices; we'll only lose it for some old ISA >> drivers. >> >> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> > > Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Applied. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation 2012-01-16 22:38 [PATCH net 1/2] net: Use device model to get driver name in skb_gso_segment() Ben Hutchings 2012-01-16 22:41 ` Herbert Xu @ 2012-01-16 22:43 ` Ben Hutchings 2012-01-17 6:11 ` Herbert Xu 1 sibling, 1 reply; 7+ messages in thread From: Ben Hutchings @ 2012-01-16 22:43 UTC (permalink / raw) To: David Miller; +Cc: Herbert Xu, netdev skb_checksum_help() has never done anything useful with skbs that require segmentation. Setting skb->ip_summed = CHECKSUM_NONE makes them invalid and provokes a later WARNing in skb_gso_segment(). Passing such an skb to skb_checksum_help() indicates a bug, so we should warn about it immediately. Move the warning from skb_gso_segment() into a shared function, and add the calling function name, gso_type and gso_size to it. Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> --- The price for writing the warning format only once is having to pass in the calling function name. Not sure whether it's a good trade-off. Ben. net/core/dev.c | 31 +++++++++++++++++++------------ 1 files changed, 19 insertions(+), 12 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 7e6b7dc..6824393 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1887,6 +1887,22 @@ void skb_set_dev(struct sk_buff *skb, struct net_device *dev) EXPORT_SYMBOL(skb_set_dev); #endif /* CONFIG_NET_NS */ +static void skb_warn_bad_offload(const char *func, const struct sk_buff *skb) +{ + struct net_device *dev = skb->dev; + const char *driver = ""; + + if (dev && dev->dev.parent) + driver = dev_driver_string(dev->dev.parent); + + WARN(1, "%s: in %s caps=(%pNF, %pNF) len=%d data_len=%d gso_size=%d " + "gso_type=%d ip_summed=%d\n", + driver, func, dev ? &dev->features : NULL, + skb->sk ? &skb->sk->sk_route_caps : NULL, + skb->len, skb->data_len, skb_shinfo(skb)->gso_size, + skb_shinfo(skb)->gso_type, skb->ip_summed); +} + /* * Invalidate hardware checksum when packet is to be mangled, and * complete checksum manually on outgoing path. @@ -1900,8 +1916,8 @@ int skb_checksum_help(struct sk_buff *skb) goto out_set_summed; if (unlikely(skb_shinfo(skb)->gso_size)) { - /* Let GSO fix up the checksum. */ - goto out_set_summed; + skb_warn_bad_offload(__func__, skb); + return -EINVAL; } offset = skb_checksum_start_offset(skb); @@ -1961,16 +1977,7 @@ struct sk_buff *skb_gso_segment(struct sk_buff *skb, __skb_pull(skb, skb->mac_len); if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { - struct net_device *dev = skb->dev; - const char *driver = ""; - - if (dev && dev->dev.parent) - driver = dev_driver_string(dev->dev.parent); - - WARN(1, "%s: caps=(%pNF, %pNF) len=%d data_len=%d ip_summed=%d\n", - driver, dev ? &dev->features : NULL, - skb->sk ? &skb->sk->sk_route_caps : NULL, - skb->len, skb->data_len, skb->ip_summed); + skb_warn_bad_offload(__func__, skb); if (skb_header_cloned(skb) && (err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC))) -- 1.7.7.5 -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation 2012-01-16 22:43 ` [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation Ben Hutchings @ 2012-01-17 6:11 ` Herbert Xu 2012-01-17 15:32 ` David Miller 2012-01-17 17:15 ` Ben Hutchings 0 siblings, 2 replies; 7+ messages in thread From: Herbert Xu @ 2012-01-17 6:11 UTC (permalink / raw) To: Ben Hutchings; +Cc: David Miller, netdev On Mon, Jan 16, 2012 at 10:43:38PM +0000, Ben Hutchings wrote: > skb_checksum_help() has never done anything useful with skbs that > require segmentation. Setting skb->ip_summed = CHECKSUM_NONE makes > them invalid and provokes a later WARNing in skb_gso_segment(). > > Passing such an skb to skb_checksum_help() indicates a bug, so we > should warn about it immediately. Move the warning from > skb_gso_segment() into a shared function, and add the calling function > name, gso_type and gso_size to it. > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> > --- > The price for writing the warning format only once is having to pass in > the calling function name. Not sure whether it's a good trade-off. Do we really need the name since we should get a back trace? 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] 7+ messages in thread
* Re: [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation 2012-01-17 6:11 ` Herbert Xu @ 2012-01-17 15:32 ` David Miller 2012-01-17 17:15 ` Ben Hutchings 1 sibling, 0 replies; 7+ messages in thread From: David Miller @ 2012-01-17 15:32 UTC (permalink / raw) To: herbert; +Cc: bhutchings, netdev From: Herbert Xu <herbert@gondor.hengli.com.au> Date: Tue, 17 Jan 2012 17:11:15 +1100 > On Mon, Jan 16, 2012 at 10:43:38PM +0000, Ben Hutchings wrote: >> skb_checksum_help() has never done anything useful with skbs that >> require segmentation. Setting skb->ip_summed = CHECKSUM_NONE makes >> them invalid and provokes a later WARNing in skb_gso_segment(). >> >> Passing such an skb to skb_checksum_help() indicates a bug, so we >> should warn about it immediately. Move the warning from >> skb_gso_segment() into a shared function, and add the calling function >> name, gso_type and gso_size to it. >> >> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> >> --- >> The price for writing the warning format only once is having to pass in >> the calling function name. Not sure whether it's a good trade-off. > > Do we really need the name since we should get a back trace? Agreed. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation 2012-01-17 6:11 ` Herbert Xu 2012-01-17 15:32 ` David Miller @ 2012-01-17 17:15 ` Ben Hutchings 1 sibling, 0 replies; 7+ messages in thread From: Ben Hutchings @ 2012-01-17 17:15 UTC (permalink / raw) To: Herbert Xu; +Cc: David Miller, netdev On Tue, 2012-01-17 at 17:11 +1100, Herbert Xu wrote: > On Mon, Jan 16, 2012 at 10:43:38PM +0000, Ben Hutchings wrote: > > skb_checksum_help() has never done anything useful with skbs that > > require segmentation. Setting skb->ip_summed = CHECKSUM_NONE makes > > them invalid and provokes a later WARNing in skb_gso_segment(). > > > > Passing such an skb to skb_checksum_help() indicates a bug, so we > > should warn about it immediately. Move the warning from > > skb_gso_segment() into a shared function, and add the calling function > > name, gso_type and gso_size to it. > > > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> > > --- > > The price for writing the warning format only once is having to pass in > > the calling function name. Not sure whether it's a good trade-off. > > Do we really need the name since we should get a back trace? If the compiler were to aggressively inline some of these functions then it might no longer be clear where this was called from. But I agree that the name is normally redundant, so I'll take it out. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-01-17 17:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-16 22:38 [PATCH net 1/2] net: Use device model to get driver name in skb_gso_segment() Ben Hutchings 2012-01-16 22:41 ` Herbert Xu 2012-01-17 15:32 ` David Miller 2012-01-16 22:43 ` [PATCH net 2/2] net: WARN if skb_checksum_help() is called on skb requiring segmentation Ben Hutchings 2012-01-17 6:11 ` Herbert Xu 2012-01-17 15:32 ` David Miller 2012-01-17 17:15 ` Ben Hutchings
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).