| Message ID | 20211118160301.GA19542@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v7] Add payload to be 32-bit aligned to fix dropped packets | expand |
On 11/18/21 8:03 AM, Kumar Thangavel wrote: > Update NC-SI command handler (both standard and OEM) to take into > account of payload paddings in allocating skb (in case of payload > size is not 32-bit aligned). > > The checksum field follows payload field, without taking payload > padding into account can cause checksum being truncated, leading to > dropped packets. > Patch title should start with a prefix, identifying which layer/driver is involved. Probably in this case net/ncsi: Add payload to be 32-bit aligned to fix dropped packets > Fixes: fb4ee67529ff ("net/ncsi: Add NCSI OEM command support") > Signed-off-by: Kumar Thangavel <thangavel.k@hcl.com> > Acked-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> > > --- > v7: > - Updated padding_bytes as const static int variable > > v6: > - Updated type of padding_bytes variable > - Updated type of payload > - Seperated variable declarations and code > > v5: > - Added Fixes tag > - Added const variable for padding_bytes > > v4: > - Used existing macro for max function > > v3: > - Added Macro for MAX > - Fixed the missed semicolon > > v2: > - Added NC-SI spec version and section > - Removed blank line > - corrected spellings > > v1: > - Initial draft > > --- > --- > net/ncsi/ncsi-cmd.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c > index ba9ae482141b..9a6f10f4833e 100644 > --- a/net/ncsi/ncsi-cmd.c > +++ b/net/ncsi/ncsi-cmd.c > @@ -18,6 +18,8 @@ > #include "internal.h" > #include "ncsi-pkt.h" > > +const static int padding_bytes = 26; It would be nice to have some kind of reference, to explain what is this magic value. Moving the comment [1] here might make sense. > + > u32 ncsi_calculate_checksum(unsigned char *data, int len) > { > u32 checksum = 0; > @@ -213,12 +215,17 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb, > { > struct ncsi_cmd_oem_pkt *cmd; > unsigned int len; > + int payload; [1] This comment could be moved before @padding_bytes > + /* NC-SI spec DSP_0222_1.2.0, section 8.2.2.2 > + * requires payload to be padded with 0 to > + * 32-bit boundary before the checksum field. > + * Ensure the padding bytes are accounted for in > + * skb allocation > + */ > > + payload = ALIGN(nca->payload, 4); > len = sizeof(struct ncsi_cmd_pkt_hdr) + 4; > - if (nca->payload < 26) > - len += 26; > - else > - len += nca->payload; > + len += max(payload, padding_bytes); > > cmd = skb_put_zero(skb, len); > memcpy(&cmd->mfr_id, nca->data, nca->payload); > @@ -272,6 +279,7 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) > struct net_device *dev = nd->dev; > int hlen = LL_RESERVED_SPACE(dev); > int tlen = dev->needed_tailroom; > + int payload; > int len = hlen + tlen; > struct sk_buff *skb; > struct ncsi_request *nr; > @@ -281,14 +289,14 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) > return NULL; > > /* NCSI command packet has 16-bytes header, payload, 4 bytes checksum. > + * Payload needs padding so that the checksum field following payload is > + * aligned to 32-bit boundary. > * The packet needs padding if its payload is less than 26 bytes to > * meet 64 bytes minimal ethernet frame length. > */ > len += sizeof(struct ncsi_cmd_pkt_hdr) + 4; > - if (nca->payload < 26) > - len += 26; > - else > - len += nca->payload; > + payload = ALIGN(nca->payload, 4); > + len += max(payload, padding_bytes); > > /* Allocate skb */ > skb = alloc_skb(len, GFP_ATOMIC); >
Hi Kumar,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.16-rc1 next-20211118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kumar-Thangavel/Add-payload-to-be-32-bit-aligned-to-fix-dropped-packets/20211119-000541
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 42eb8fdac2fc5d62392dcfcf0253753e821a97b0
config: m68k-randconfig-r005-20211118 (attached as .config)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/f4e0a51f25ba398ad7f2e3efa5076b2e0ad24e3b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Kumar-Thangavel/Add-payload-to-be-32-bit-aligned-to-fix-dropped-packets/20211119-000541
git checkout f4e0a51f25ba398ad7f2e3efa5076b2e0ad24e3b
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> net/ncsi/ncsi-cmd.c:21:1: warning: 'static' is not at beginning of declaration [-Wold-style-declaration]
21 | const static int padding_bytes = 26;
| ^~~~~
vim +/static +21 net/ncsi/ncsi-cmd.c
20
> 21 const static int padding_bytes = 26;
22
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, 18 Nov 2021 21:33:02 +0530 Kumar Thangavel wrote:
> +const static int padding_bytes = 26;
/net/ncsi/ncsi-cmd.c:21:1: warning: ‘static’ is not at beginning of declaration [-Wold-style-declaration]
21 | const static int padding_bytes = 26;
| ^~~~~
On Fri, Nov 19, 2021 at 12:02 PM Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 18 Nov 2021 21:33:02 +0530 Kumar Thangavel wrote: > > +const static int padding_bytes = 26; > > /net/ncsi/ncsi-cmd.c:21:1: warning: ‘static’ is not at beginning of > declaration [-Wold-style-declaration] > 21 | const static int padding_bytes = 26; > ACK. > | ^~~~~ >
On Thu, Nov 18, 2021 at 10:44 PM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > On 11/18/21 8:03 AM, Kumar Thangavel wrote: > > Update NC-SI command handler (both standard and OEM) to take into > > account of payload paddings in allocating skb (in case of payload > > size is not 32-bit aligned). > > > > The checksum field follows payload field, without taking payload > > padding into account can cause checksum being truncated, leading to > > dropped packets. > > > > Patch title should start with a prefix, identifying which layer/driver is > involved. > > Probably in this case > > net/ncsi: Add payload to be 32-bit aligned to fix dropped packets > ACK. > > > Fixes: fb4ee67529ff ("net/ncsi: Add NCSI OEM command support") > > Signed-off-by: Kumar Thangavel <thangavel.k@hcl.com> > > Acked-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> > > > > --- > > v7: > > - Updated padding_bytes as const static int variable > > > > v6: > > - Updated type of padding_bytes variable > > - Updated type of payload > > - Seperated variable declarations and code > > > > v5: > > - Added Fixes tag > > - Added const variable for padding_bytes > > > > v4: > > - Used existing macro for max function > > > > v3: > > - Added Macro for MAX > > - Fixed the missed semicolon > > > > v2: > > - Added NC-SI spec version and section > > - Removed blank line > > - corrected spellings > > > > v1: > > - Initial draft > > > > --- > > --- > > net/ncsi/ncsi-cmd.c | 24 ++++++++++++++++-------- > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c > > index ba9ae482141b..9a6f10f4833e 100644 > > --- a/net/ncsi/ncsi-cmd.c > > +++ b/net/ncsi/ncsi-cmd.c > > @@ -18,6 +18,8 @@ > > #include "internal.h" > > #include "ncsi-pkt.h" > > > > +const static int padding_bytes = 26; > > It would be nice to have some kind of reference, to explain > what is this magic value. > > ACK. > Moving the comment [1] here might make sense. > > > > + > > u32 ncsi_calculate_checksum(unsigned char *data, int len) > > { > > u32 checksum = 0; > > @@ -213,12 +215,17 @@ static int ncsi_cmd_handler_oem(struct sk_buff > *skb, > > { > > struct ncsi_cmd_oem_pkt *cmd; > > unsigned int len; > > + int payload; > > [1] This comment could be moved before @padding_bytes > > > + /* NC-SI spec DSP_0222_1.2.0, section 8.2.2.2 > > + * requires payload to be padded with 0 to > > + * 32-bit boundary before the checksum field. > > + * Ensure the padding bytes are accounted for in > > + * skb allocation > > + */ > > > > + payload = ALIGN(nca->payload, 4); > > len = sizeof(struct ncsi_cmd_pkt_hdr) + 4; > > - if (nca->payload < 26) > > - len += 26; > > - else > > - len += nca->payload; > > + len += max(payload, padding_bytes); > > > > cmd = skb_put_zero(skb, len); > > memcpy(&cmd->mfr_id, nca->data, nca->payload); > > @@ -272,6 +279,7 @@ static struct ncsi_request > *ncsi_alloc_command(struct ncsi_cmd_arg *nca) > > struct net_device *dev = nd->dev; > > int hlen = LL_RESERVED_SPACE(dev); > > int tlen = dev->needed_tailroom; > > + int payload; > > int len = hlen + tlen; > > struct sk_buff *skb; > > struct ncsi_request *nr; > > @@ -281,14 +289,14 @@ static struct ncsi_request > *ncsi_alloc_command(struct ncsi_cmd_arg *nca) > > return NULL; > > > > /* NCSI command packet has 16-bytes header, payload, 4 bytes > checksum. > > + * Payload needs padding so that the checksum field following > payload is > > + * aligned to 32-bit boundary. > > * The packet needs padding if its payload is less than 26 bytes to > > * meet 64 bytes minimal ethernet frame length. > > */ > > len += sizeof(struct ncsi_cmd_pkt_hdr) + 4; > > - if (nca->payload < 26) > > - len += 26; > > - else > > - len += nca->payload; > > + payload = ALIGN(nca->payload, 4); > > + len += max(payload, padding_bytes); > > > > /* Allocate skb */ > > skb = alloc_skb(len, GFP_ATOMIC); > > >
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c index ba9ae482141b..9a6f10f4833e 100644 --- a/net/ncsi/ncsi-cmd.c +++ b/net/ncsi/ncsi-cmd.c @@ -18,6 +18,8 @@ #include "internal.h" #include "ncsi-pkt.h" +const static int padding_bytes = 26; + u32 ncsi_calculate_checksum(unsigned char *data, int len) { u32 checksum = 0; @@ -213,12 +215,17 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb, { struct ncsi_cmd_oem_pkt *cmd; unsigned int len; + int payload; + /* NC-SI spec DSP_0222_1.2.0, section 8.2.2.2 + * requires payload to be padded with 0 to + * 32-bit boundary before the checksum field. + * Ensure the padding bytes are accounted for in + * skb allocation + */ + payload = ALIGN(nca->payload, 4); len = sizeof(struct ncsi_cmd_pkt_hdr) + 4; - if (nca->payload < 26) - len += 26; - else - len += nca->payload; + len += max(payload, padding_bytes); cmd = skb_put_zero(skb, len); memcpy(&cmd->mfr_id, nca->data, nca->payload); @@ -272,6 +279,7 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) struct net_device *dev = nd->dev; int hlen = LL_RESERVED_SPACE(dev); int tlen = dev->needed_tailroom; + int payload; int len = hlen + tlen; struct sk_buff *skb; struct ncsi_request *nr; @@ -281,14 +289,14 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) return NULL; /* NCSI command packet has 16-bytes header, payload, 4 bytes checksum. + * Payload needs padding so that the checksum field following payload is + * aligned to 32-bit boundary. * The packet needs padding if its payload is less than 26 bytes to * meet 64 bytes minimal ethernet frame length. */ len += sizeof(struct ncsi_cmd_pkt_hdr) + 4; - if (nca->payload < 26) - len += 26; - else - len += nca->payload; + payload = ALIGN(nca->payload, 4); + len += max(payload, padding_bytes); /* Allocate skb */ skb = alloc_skb(len, GFP_ATOMIC);
Update NC-SI command handler (both standard and OEM) to take into account of payload paddings in allocating skb (in case of payload size is not 32-bit aligned). The checksum field follows payload field, without taking payload padding into account can cause checksum being truncated, leading to dropped packets. Fixes: fb4ee67529ff ("net/ncsi: Add NCSI OEM command support") Signed-off-by: Kumar Thangavel <thangavel.k@hcl.com> Acked-by: Samuel Mendoza-Jonas <sam@mendozajonas.com> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> --- v7: - Updated padding_bytes as const static int variable v6: - Updated type of padding_bytes variable - Updated type of payload - Seperated variable declarations and code v5: - Added Fixes tag - Added const variable for padding_bytes v4: - Used existing macro for max function v3: - Added Macro for MAX - Fixed the missed semicolon v2: - Added NC-SI spec version and section - Removed blank line - corrected spellings v1: - Initial draft --- --- net/ncsi/ncsi-cmd.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)