| Message ID | 1304378957-24123-2-git-send-email-anirban.chakraborty@qlogic.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote: > From: Anirban Chakraborty <anirban.chakraborty@qlogic.com> > > Added code to take FW dump via ethtool. A pair of set and get functions are > added to configure dump level and fetch it from the driver respectively. A > third function is added to retrieve the dumped FW data from the driver. > > Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com> > --- > include/linux/ethtool.h | 20 ++++++++++++ > net/core/ethtool.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 96 insertions(+), 0 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 9de3127..3dd91a5 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -595,6 +595,19 @@ struct ethtool_flash { > char data[ETHTOOL_FLASH_MAX_FILENAME]; > }; > > +/** > + * struct ethtool_dump - used for retrieving, setting device dump > + * @flag: flag for dump setting > + * @len: length of dump data > + * @data: data collected for this command > + */ > +struct ethtool_dump { > + __u32 cmd; > + __u32 flag; > + __u32 len; > + u8 data[0]; > +}; > + What are the legal values of flags? Are they expected to be entirely driver-dependent? Shouldn't there be a version field, as for register dumps? > /* for returning and changing feature sets */ > > /** > @@ -926,6 +939,10 @@ struct ethtool_ops { > const struct ethtool_rxfh_indir *); > void (*get_channels)(struct net_device *, struct ethtool_channels *); > int (*set_channels)(struct net_device *, struct ethtool_channels *); > + int (*set_dump)(struct net_device *, struct ethtool_dump *); > + int (*get_dump)(struct net_device *, struct ethtool_dump *); > + int (*get_dump_data)(struct net_device *, > + struct ethtool_dump *, void *); These need to be properly documented in the header comment. > }; > #endif /* __KERNEL__ */ > @@ -997,6 +1014,9 @@ struct ethtool_ops { > #define ETHTOOL_SFEATURES 0x0000003b /* Change device offload settings */ > #define ETHTOOL_GCHANNELS 0x0000003c /* Get no of channels */ > #define ETHTOOL_SCHANNELS 0x0000003d /* Set no of channels */ > +#define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ > +#define ETHTOOL_GET_DUMP 0x0000003f /* Get dump settings */ > +#define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ > > /* compatibility with older code */ > #define SPARC_ETH_GSET ETHTOOL_GSET > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index d8b1a8d..dce547c 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev, > return dev->ethtool_ops->flash_device(dev, &efl); > } > > +static int ethtool_set_dump(struct net_device *dev, > + void __user *useraddr) > +{ > + struct ethtool_dump dump; > + > + if (!dev->ethtool_ops->set_dump) > + return -EOPNOTSUPP; > + > + if (copy_from_user(&dump, useraddr, sizeof(dump))) > + return -EFAULT; > + > + return dev->ethtool_ops->set_dump(dev, &dump); > +} > + > +static int ethtool_get_dump(struct net_device *dev, > + void __user *useraddr) > +{ > + struct ethtool_dump dump; > + const struct ethtool_ops *ops = dev->ethtool_ops; > + > + if (!dev->ethtool_ops->get_dump) > + return -EOPNOTSUPP; > + > + if (copy_from_user(&dump, useraddr, sizeof(dump))) > + return -EFAULT; > + > + if (ops->get_dump(dev, &dump)) > + return -EFAULT; Why does this change the error code? > + if (copy_to_user(useraddr, &dump, sizeof(dump))) > + return -EFAULT; > + return 0; > +} > + > +static int ethtool_get_dump_data(struct net_device *dev, > + void __user *useraddr) > +{ > + int ret; > + void *data; > + struct ethtool_dump dump; > + const struct ethtool_ops *ops = dev->ethtool_ops; > + > + if (!dev->ethtool_ops->get_dump_data) > + return -EOPNOTSUPP; > + > + if (copy_from_user(&dump, useraddr, sizeof(dump))) > + return -EFAULT; > + data = vzalloc(dump.len); I think we ought to query the driver to find out the maximum dump length, rather than using the length passed by the user (up to 4GB). We already check that the user has the CAP_NET_ADMIN capability but that should not mean the ability to evade memory limits. > + if (!data) > + return -ENOMEM; > + ret = ops->get_dump_data(dev, &dump, data); > + if (ret) { > + ret = -EFAULT; Why does this change the error code? > + goto out; > + } > + if (copy_to_user(useraddr, &dump, sizeof(dump))) { > + ret = -EFAULT; > + goto out; > + } > + useraddr += offsetof(struct ethtool_dump, data); > + if (copy_to_user(useraddr, data, dump.len)) > + ret = -EFAULT; > +out: > + vfree(data); > + return ret; > +} > + > /* The main entry point in this file. Called from net/core/dev.c */ > > int dev_ethtool(struct net *net, struct ifreq *ifr) > @@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > case ETHTOOL_SCHANNELS: > rc = ethtool_set_channels(dev, useraddr); > break; > + case ETHTOOL_SET_DUMP: > + rc = ethtool_set_dump(dev, useraddr); > + break; > + case ETHTOOL_GET_DUMP: > + rc = ethtool_get_dump(dev, useraddr); > + break; > + case ETHTOOL_GET_DUMP_DATA: > + rc = ethtool_get_dump_data(dev, useraddr); > + break; > default: > rc = -EOPNOTSUPP; > }
On May 3, 2011, at 4:53 PM, Ben Hutchings wrote: > On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote: >> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com> >> >> Added code to take FW dump via ethtool. A pair of set and get functions are >> added to configure dump level and fetch it from the driver respectively. A >> third function is added to retrieve the dumped FW data from the driver. >> >> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com> >> --- >> include/linux/ethtool.h | 20 ++++++++++++ >> net/core/ethtool.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 96 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index 9de3127..3dd91a5 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -595,6 +595,19 @@ struct ethtool_flash { >> char data[ETHTOOL_FLASH_MAX_FILENAME]; >> }; >> >> +/** >> + * struct ethtool_dump - used for retrieving, setting device dump >> + * @flag: flag for dump setting >> + * @len: length of dump data >> + * @data: data collected for this command >> + */ >> +struct ethtool_dump { >> + __u32 cmd; >> + __u32 flag; >> + __u32 len; >> + u8 data[0]; >> +}; >> + > > What are the legal values of flags? Are they expected to be entirely > driver-dependent? Yes, the flag could be completely driver dependent. For example, in the qlcnic driver this would indicate the level of the dump that the driver should take. Also, by specifying a magic key via the flag, the driver could be instructed to take a FW dump forcibly. > > Shouldn't there be a version field, as for register dumps? I was thinking that the dump data itself could contain the version field specifying the FW version. However, we can have an additional field if that turns out to be useful. > >> /* for returning and changing feature sets */ >> >> /** >> @@ -926,6 +939,10 @@ struct ethtool_ops { >> const struct ethtool_rxfh_indir *); >> void (*get_channels)(struct net_device *, struct ethtool_channels *); >> int (*set_channels)(struct net_device *, struct ethtool_channels *); >> + int (*set_dump)(struct net_device *, struct ethtool_dump *); >> + int (*get_dump)(struct net_device *, struct ethtool_dump *); >> + int (*get_dump_data)(struct net_device *, >> + struct ethtool_dump *, void *); > > These need to be properly documented in the header comment. WIll do. > >> }; >> #endif /* __KERNEL__ */ >> @@ -997,6 +1014,9 @@ struct ethtool_ops { >> #define ETHTOOL_SFEATURES 0x0000003b /* Change device offload settings */ >> #define ETHTOOL_GCHANNELS 0x0000003c /* Get no of channels */ >> #define ETHTOOL_SCHANNELS 0x0000003d /* Set no of channels */ >> +#define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ >> +#define ETHTOOL_GET_DUMP 0x0000003f /* Get dump settings */ >> +#define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ >> >> /* compatibility with older code */ >> #define SPARC_ETH_GSET ETHTOOL_GSET >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >> index d8b1a8d..dce547c 100644 >> --- a/net/core/ethtool.c >> +++ b/net/core/ethtool.c >> @@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev, >> return dev->ethtool_ops->flash_device(dev, &efl); >> } >> >> +static int ethtool_set_dump(struct net_device *dev, >> + void __user *useraddr) >> +{ >> + struct ethtool_dump dump; >> + >> + if (!dev->ethtool_ops->set_dump) >> + return -EOPNOTSUPP; >> + >> + if (copy_from_user(&dump, useraddr, sizeof(dump))) >> + return -EFAULT; >> + >> + return dev->ethtool_ops->set_dump(dev, &dump); >> +} >> + >> +static int ethtool_get_dump(struct net_device *dev, >> + void __user *useraddr) >> +{ >> + struct ethtool_dump dump; >> + const struct ethtool_ops *ops = dev->ethtool_ops; >> + >> + if (!dev->ethtool_ops->get_dump) >> + return -EOPNOTSUPP; >> + >> + if (copy_from_user(&dump, useraddr, sizeof(dump))) >> + return -EFAULT; >> + >> + if (ops->get_dump(dev, &dump)) >> + return -EFAULT; > > Why does this change the error code? It should not. Will fix. > >> + if (copy_to_user(useraddr, &dump, sizeof(dump))) >> + return -EFAULT; >> + return 0; >> +} >> + >> +static int ethtool_get_dump_data(struct net_device *dev, >> + void __user *useraddr) >> +{ >> + int ret; >> + void *data; >> + struct ethtool_dump dump; >> + const struct ethtool_ops *ops = dev->ethtool_ops; >> + >> + if (!dev->ethtool_ops->get_dump_data) >> + return -EOPNOTSUPP; >> + >> + if (copy_from_user(&dump, useraddr, sizeof(dump))) >> + return -EFAULT; >> + data = vzalloc(dump.len); > > I think we ought to query the driver to find out the maximum dump > length, rather than using the length passed by the user (up to 4GB). We > already check that the user has the CAP_NET_ADMIN capability but that > should not mean the ability to evade memory limits. That's right. The user makes a call to get_dump (ETHTOOL_GET_DUMP) first to get the size of the collected dump data for the current dump level (flag). Then it makes the call to get the dump data (ETHTOOL_GET_DUMP_DATA), as in above where it specifies the dump size. The patch ref-ed below demonstrated that. http://patchwork.ozlabs.org/patch/93729/ > >> + if (!data) >> + return -ENOMEM; >> + ret = ops->get_dump_data(dev, &dump, data); >> + if (ret) { >> + ret = -EFAULT; > > Why does this change the error code? Will fix it. > >> + goto out; >> + } >> + if (copy_to_user(useraddr, &dump, sizeof(dump))) { >> + ret = -EFAULT; >> + goto out; >> + } >> + useraddr += offsetof(struct ethtool_dump, data); >> + if (copy_to_user(useraddr, data, dump.len)) >> + ret = -EFAULT; >> +out: >> + vfree(data); >> + return ret; >> +} >> + >> /* The main entry point in this file. Called from net/core/dev.c */ >> >> int dev_ethtool(struct net *net, struct ifreq *ifr) >> @@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) >> case ETHTOOL_SCHANNELS: >> rc = ethtool_set_channels(dev, useraddr); >> break; >> + case ETHTOOL_SET_DUMP: >> + rc = ethtool_set_dump(dev, useraddr); >> + break; >> + case ETHTOOL_GET_DUMP: >> + rc = ethtool_get_dump(dev, useraddr); >> + break; >> + case ETHTOOL_GET_DUMP_DATA: >> + rc = ethtool_get_dump_data(dev, useraddr); >> + break; >> default: >> rc = -EOPNOTSUPP; >> } > Thanks for your feedback. I'll respin the patch incorporating your comments and submit it again. -Anirban -- 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 Tue, 2011-05-03 at 17:29 -0700, Anirban Chakraborty wrote: > On May 3, 2011, at 4:53 PM, Ben Hutchings wrote: > > > On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote: [...] > >> +static int ethtool_get_dump_data(struct net_device *dev, > >> + void __user *useraddr) > >> +{ > >> + int ret; > >> + void *data; > >> + struct ethtool_dump dump; > >> + const struct ethtool_ops *ops = dev->ethtool_ops; > >> + > >> + if (!dev->ethtool_ops->get_dump_data) > >> + return -EOPNOTSUPP; > >> + > >> + if (copy_from_user(&dump, useraddr, sizeof(dump))) > >> + return -EFAULT; > >> + data = vzalloc(dump.len); > > > > I think we ought to query the driver to find out the maximum dump > > length, rather than using the length passed by the user (up to 4GB). We > > already check that the user has the CAP_NET_ADMIN capability but that > > should not mean the ability to evade memory limits. > That's right. The user makes a call to get_dump (ETHTOOL_GET_DUMP) > first to get the size of the collected dump data for the current > dump level (flag). Then it makes the call to get the dump data (ETHTOOL_GET_DUMP_DATA), > as in above where it specifies the dump size. > The patch ref-ed below demonstrated that. > http://patchwork.ozlabs.org/patch/93729/ [...] I understand that the userland application will need to get the size that way. I'm saying that this code in the kernel should also get the size from the driver, so that a malicious application cannot make the kernel allocate an excessively large buffer. Also, you'll need to submit your implementation along with this, as David won't accept an extension to the API without a driver that implements it. Ben.
On May 3, 2011, at 6:06 PM, Ben Hutchings wrote: > <snip> > > I understand that the userland application will need to get the size > that way. I'm saying that this code in the kernel should also get the > size from the driver, so that a malicious application cannot make the > kernel allocate an excessively large buffer. Makes sense. Will do so. > > Also, you'll need to submit your implementation along with this, as > David won't accept an extension to the API without a driver that > implements it. I have implemented it in qlcnic driver. Will submit it along with the v2 patches. Thanks. -Anirban -- 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/include/linux/ethtool.h b/include/linux/ethtool.h index 9de3127..3dd91a5 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -595,6 +595,19 @@ struct ethtool_flash { char data[ETHTOOL_FLASH_MAX_FILENAME]; }; +/** + * struct ethtool_dump - used for retrieving, setting device dump + * @flag: flag for dump setting + * @len: length of dump data + * @data: data collected for this command + */ +struct ethtool_dump { + __u32 cmd; + __u32 flag; + __u32 len; + u8 data[0]; +}; + /* for returning and changing feature sets */ /** @@ -926,6 +939,10 @@ struct ethtool_ops { const struct ethtool_rxfh_indir *); void (*get_channels)(struct net_device *, struct ethtool_channels *); int (*set_channels)(struct net_device *, struct ethtool_channels *); + int (*set_dump)(struct net_device *, struct ethtool_dump *); + int (*get_dump)(struct net_device *, struct ethtool_dump *); + int (*get_dump_data)(struct net_device *, + struct ethtool_dump *, void *); }; #endif /* __KERNEL__ */ @@ -997,6 +1014,9 @@ struct ethtool_ops { #define ETHTOOL_SFEATURES 0x0000003b /* Change device offload settings */ #define ETHTOOL_GCHANNELS 0x0000003c /* Get no of channels */ #define ETHTOOL_SCHANNELS 0x0000003d /* Set no of channels */ +#define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */ +#define ETHTOOL_GET_DUMP 0x0000003f /* Get dump settings */ +#define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */ /* compatibility with older code */ #define SPARC_ETH_GSET ETHTOOL_GSET diff --git a/net/core/ethtool.c b/net/core/ethtool.c index d8b1a8d..dce547c 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev, return dev->ethtool_ops->flash_device(dev, &efl); } +static int ethtool_set_dump(struct net_device *dev, + void __user *useraddr) +{ + struct ethtool_dump dump; + + if (!dev->ethtool_ops->set_dump) + return -EOPNOTSUPP; + + if (copy_from_user(&dump, useraddr, sizeof(dump))) + return -EFAULT; + + return dev->ethtool_ops->set_dump(dev, &dump); +} + +static int ethtool_get_dump(struct net_device *dev, + void __user *useraddr) +{ + struct ethtool_dump dump; + const struct ethtool_ops *ops = dev->ethtool_ops; + + if (!dev->ethtool_ops->get_dump) + return -EOPNOTSUPP; + + if (copy_from_user(&dump, useraddr, sizeof(dump))) + return -EFAULT; + + if (ops->get_dump(dev, &dump)) + return -EFAULT; + + if (copy_to_user(useraddr, &dump, sizeof(dump))) + return -EFAULT; + return 0; +} + +static int ethtool_get_dump_data(struct net_device *dev, + void __user *useraddr) +{ + int ret; + void *data; + struct ethtool_dump dump; + const struct ethtool_ops *ops = dev->ethtool_ops; + + if (!dev->ethtool_ops->get_dump_data) + return -EOPNOTSUPP; + + if (copy_from_user(&dump, useraddr, sizeof(dump))) + return -EFAULT; + data = vzalloc(dump.len); + if (!data) + return -ENOMEM; + ret = ops->get_dump_data(dev, &dump, data); + if (ret) { + ret = -EFAULT; + goto out; + } + if (copy_to_user(useraddr, &dump, sizeof(dump))) { + ret = -EFAULT; + goto out; + } + useraddr += offsetof(struct ethtool_dump, data); + if (copy_to_user(useraddr, data, dump.len)) + ret = -EFAULT; +out: + vfree(data); + return ret; +} + /* The main entry point in this file. Called from net/core/dev.c */ int dev_ethtool(struct net *net, struct ifreq *ifr) @@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) case ETHTOOL_SCHANNELS: rc = ethtool_set_channels(dev, useraddr); break; + case ETHTOOL_SET_DUMP: + rc = ethtool_set_dump(dev, useraddr); + break; + case ETHTOOL_GET_DUMP: + rc = ethtool_get_dump(dev, useraddr); + break; + case ETHTOOL_GET_DUMP_DATA: + rc = ethtool_get_dump_data(dev, useraddr); + break; default: rc = -EOPNOTSUPP; }