Message ID | 1411063227.7106.280.camel@edumazet-glaptop2.roam.corp.google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, 2014-09-18 at 11:00 -0700, Eric Dumazet wrote: > On Thu, 2014-09-18 at 09:32 -0700, Eric Dumazet wrote: > > On Thu, 2014-09-18 at 09:26 -0700, Stephen Hemminger wrote: [] > > Or, do you have an idea ? > > Seems straightforward ... > > Or can you carry this fix for me ? > > Thanks > > [PATCH] ipoib: validate struct ipoib_cb size > > To catch future errors sooner. [] > diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h [] > +static inline struct ipoib_cb *ipoib_skb_cb(const struct sk_buff *skb) > +{ > + BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct ipoib_cb)); > + return (struct ipoib_cb *)skb->cb; > +} It seems better not to use const for the struct sk_buff * here. Neither of the uses take a const struct sk_buff * > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c > @@ -716,7 +716,7 @@ static int ipoib_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct ipoib_dev_priv *priv = netdev_priv(dev); > struct ipoib_neigh *neigh; > - struct ipoib_cb *cb = (struct ipoib_cb *) skb->cb; > + struct ipoib_cb *cb = ipoib_skb_cb(skb); > struct ipoib_header *header; > unsigned long flags; > > @@ -813,7 +813,7 @@ static int ipoib_hard_header(struct sk_buff *skb, > const void *daddr, const void *saddr, unsigned len) > { > struct ipoib_header *header; > - struct ipoib_cb *cb = (struct ipoib_cb *) skb->cb; > + struct ipoib_cb *cb = ipoib_skb_cb(skb); > > header = (struct ipoib_header *) skb_push(skb, sizeof *header); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-09-18 at 11:07 -0700, Joe Perches wrote: > > +static inline struct ipoib_cb *ipoib_skb_cb(const struct sk_buff *skb) > > +{ > > + BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct ipoib_cb)); > > + return (struct ipoib_cb *)skb->cb; > > +} > > It seems better not to use const for the struct sk_buff * here. > > Neither of the uses take a const struct sk_buff * Thats pretty standard, check for other similar constructs like that. static inline struct qdisc_skb_cb *qdisc_skb_cb(const struct sk_buff *skb) { return (struct qdisc_skb_cb *)skb->cb; } This allows uses of the helper when the skb is only read (has the const qual) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-09-18 at 12:14 -0700, Eric Dumazet wrote: > On Thu, 2014-09-18 at 11:07 -0700, Joe Perches wrote: > > > > +static inline struct ipoib_cb *ipoib_skb_cb(const struct sk_buff *skb) > > > +{ > > > + BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct ipoib_cb)); > > > + return (struct ipoib_cb *)skb->cb; > > > +} > > > > It seems better not to use const for the struct sk_buff * here. > > > > Neither of the uses take a const struct sk_buff * > > Thats pretty standard, check for other similar constructs like that. > > > static inline struct qdisc_skb_cb *qdisc_skb_cb(const struct sk_buff *skb) > { > return (struct qdisc_skb_cb *)skb->cb; > } > > This allows uses of the helper when the skb is only read (has the const qual) I don't mind the const argument, but casting the return to non-const seems like poor form. btw: it seems like it's 8:5 non-const to const $ grep -rP --include=*.[ch] -n "cb\s*\*\s*\w+\s*\(\s*(?:const\s+)?struct\s+sk_buff\s*\*\s*\w+\s*\)" * drivers/net/ethernet/freescale/gianfar.c:2091:static inline struct txfcb *gfar_add_fcb(struct sk_buff *skb) drivers/net/wireless/ath/ath10k/core.h:83:static inline struct ath10k_skb_cb *ATH10K_SKB_CB(struct sk_buff *skb) include/net/mrp.h:38:static inline struct mrp_skb_cb *mrp_cb(struct sk_buff *skb) include/net/ieee802154_netdev.h:235:static inline struct ieee802154_mac_cb *mac_cb(struct sk_buff *skb) include/net/ieee802154_netdev.h:240:static inline struct ieee802154_mac_cb *mac_cb_init(struct sk_buff *skb) include/net/sch_generic.h:251:static inline struct qdisc_skb_cb *qdisc_skb_cb(const struct sk_buff *skb) include/net/codel.h:95:static struct codel_skb_cb *get_codel_cb(const struct sk_buff *skb) include/net/garp.h:36:static inline struct garp_skb_cb *garp_cb(struct sk_buff *skb) net/core/dev.c:2177:static struct dev_kfree_skb_cb *get_kfree_skb_cb(const struct sk_buff *skb) net/sched/sch_choke.c:142:static inline struct choke_skb_cb *choke_skb_cb(const struct sk_buff *skb) net/sched/sch_sfb.c:95:static inline struct sfb_skb_cb *sfb_skb_cb(const struct sk_buff *skb) net/sched/sch_netem.c:171:static inline struct netem_skb_cb *netem_skb_cb(struct sk_buff *skb) net/sched/sch_sfq.c:167:static inline struct sfq_skb_cb *sfq_skb_cb(const struct sk_buff *skb) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-09-18 at 12:31 -0700, Joe Perches wrote: > I don't mind the const argument, but casting > the return to non-const seems like poor form. This is the current way to do this, even if we do not like it. C does not allow to have the same function name for different parameters, there is nothing we can do about it at this moment. -> static inline const struct ipoib_cb *ipoib_skb_cb(const struct sk_buff *skb) static inline struct ipoib_cb *ipoib_skb_cb(struct sk_buff *skb) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h index 3edce617c31b..d7562beb5423 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib.h +++ b/drivers/infiniband/ulp/ipoib/ipoib.h @@ -131,6 +131,12 @@ struct ipoib_cb { u8 hwaddr[INFINIBAND_ALEN]; }; +static inline struct ipoib_cb *ipoib_skb_cb(const struct sk_buff *skb) +{ + BUILD_BUG_ON(sizeof(skb->cb) < sizeof(struct ipoib_cb)); + return (struct ipoib_cb *)skb->cb; +} + /* Used for all multicast joins (broadcast, IPv4 mcast and IPv6 mcast) */ struct ipoib_mcast { struct ib_sa_mcmember_rec mcmember; diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 1310acf6bf92..13e6e0431592 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -716,7 +716,7 @@ static int ipoib_start_xmit(struct sk_buff *skb, struct net_device *dev) { struct ipoib_dev_priv *priv = netdev_priv(dev); struct ipoib_neigh *neigh; - struct ipoib_cb *cb = (struct ipoib_cb *) skb->cb; + struct ipoib_cb *cb = ipoib_skb_cb(skb); struct ipoib_header *header; unsigned long flags; @@ -813,7 +813,7 @@ static int ipoib_hard_header(struct sk_buff *skb, const void *daddr, const void *saddr, unsigned len) { struct ipoib_header *header; - struct ipoib_cb *cb = (struct ipoib_cb *) skb->cb; + struct ipoib_cb *cb = ipoib_skb_cb(skb); header = (struct ipoib_header *) skb_push(skb, sizeof *header);