Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code
On Fri, 14 Jul 2017, Al Viro wrote: > On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote: > > > Thanks for testing it! > > > > That means we did not copy any data and the kernel continues with > > an uninitialized buffer, right? The problem may be the definition of > > > > struct kib_immediate_msg { > > struct lnet_hdr ibim_hdr;/* portals header */ > > char ibim_payload[0]; /* piggy-backed payload */ > > } WIRE_ATTR; > > > > The check that Al added will try to ensure that we don't write > > beyond the size of the ibim_payload[] array, which unfortunately > > is defined as a zero-byte array, so I can see why it will now > > fail. However, it's already broken in mainline now, with or without > > my patch. > > > > Are you able to come up with a fix that avoids the warning in > > 'allmodconfig' and makes the function do something reasonable > > again? Yes, I'm testing a fix right now which I will merge with the original patch. Greg this patch will need to be sent to Linus as well so the kernel release isn't broken for users. > Might make sense to try and use valid C99 for "array of indefinite > size as the last member", i.e. > struct kib_immediate_msg { > struct lnet_hdr ibim_hdr;/* portals header */ > char ibim_payload[]; /* piggy-backed payload */ > } WIRE_ATTR; > > Zero-sized array as the last member is gcc hack predating that; > looks like gcc gets confused into deciding that it knows the distance > from the end of object... I did some profiling and found gcc was doing the right thing. That should be updated to a C99 flexable array in a latter patch. > Said that, are we really guaranteed the IBLND_MSG_SIZE bytes > in there? This is what the real bug was. In the current code we are telling copy_from_iter and copy_to_iter that the number of bytes are always IBLND_MSG_SIZE. Arnd thought this was always the size so in his patch he was testing the returned result of copy_[from|to]_iter to IBLND_MSG_SIZE. This nearly always failed since variable sized messages are being created. The zero size I initially saw was from doing pings. When I later tested with pushing I/O packets of other sizes were observed but none of them were IBLND_MSG_SIZE in size so they failed to transmit. As soon as I'm done testing I will send a patch. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code
On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote: > Thanks for testing it! > > That means we did not copy any data and the kernel continues with > an uninitialized buffer, right? The problem may be the definition of > > struct kib_immediate_msg { > struct lnet_hdr ibim_hdr;/* portals header */ > char ibim_payload[0]; /* piggy-backed payload */ > } WIRE_ATTR; > > The check that Al added will try to ensure that we don't write > beyond the size of the ibim_payload[] array, which unfortunately > is defined as a zero-byte array, so I can see why it will now > fail. However, it's already broken in mainline now, with or without > my patch. > > Are you able to come up with a fix that avoids the warning in > 'allmodconfig' and makes the function do something reasonable > again? Might make sense to try and use valid C99 for "array of indefinite size as the last member", i.e. struct kib_immediate_msg { struct lnet_hdr ibim_hdr;/* portals header */ char ibim_payload[]; /* piggy-backed payload */ } WIRE_ATTR; Zero-sized array as the last member is gcc hack predating that; looks like gcc gets confused into deciding that it knows the distance from the end of object... Said that, are we really guaranteed the IBLND_MSG_SIZE bytes in there? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code
On Thu, Jul 13, 2017 at 7:07 PM, James Simmonswrote: > >> We now get a helpful warning for code that calls copy_{from,to}_iter >> without checking the return value, introduced by commit aa28de275a24 >> ("iov_iter/hardening: move object size checks to inlined part"). >> >> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function >> 'kiblnd_send': >> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1643:2: error: >> ignoring return value of 'copy_from_iter', declared with attribute >> warn_unused_result [-Werror=unused-result] >> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function >> 'kiblnd_recv': >> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1744:3: error: >> ignoring return value of 'copy_to_iter', declared with attribute >> warn_unused_result [-Werror=unused-result] >> >> In case we get short copies here, we may get incorrect behavior. >> I've added failure handling for both rx and tx now, returning >> -EFAULT as expected. >> >> Cc: sta...@vger.kernel.org >> Signed-off-by: Arnd Bergmann >> --- >> This warning now shows up in 'allmodconfig' builds, so it would be >> good to get it fixed quickly for 4.13, but my patch should not go >> in without careful review since I'm not familiar with with code >> and the error handling is a bit tricky here. >> >> I added 'Cc: stable' since this is a preexisting condition that we >> only started warning about now. >> --- >> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 15 +-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c >> b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c >> index 85b242ec5f9b..70256a0ffd2e 100644 >> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c >> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c >> @@ -1640,8 +1640,13 @@ kiblnd_send(struct lnet_ni *ni, void *private, struct >> lnet_msg *lntmsg) >> ibmsg = tx->tx_msg; >> ibmsg->ibm_u.immediate.ibim_hdr = *hdr; >> >> - copy_from_iter(>ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE, >> + rc = copy_from_iter(>ibm_u.immediate.ibim_payload, >> IBLND_MSG_SIZE, >> ); > > I have to Nak this to prevent it from landing but for some reason in my > testing its returning zero from copy_from_iter. Thanks for testing it! That means we did not copy any data and the kernel continues with an uninitialized buffer, right? The problem may be the definition of struct kib_immediate_msg { struct lnet_hdr ibim_hdr;/* portals header */ char ibim_payload[0]; /* piggy-backed payload */ } WIRE_ATTR; The check that Al added will try to ensure that we don't write beyond the size of the ibim_payload[] array, which unfortunately is defined as a zero-byte array, so I can see why it will now fail. However, it's already broken in mainline now, with or without my patch. Are you able to come up with a fix that avoids the warning in 'allmodconfig' and makes the function do something reasonable again? Arnd ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code
> We now get a helpful warning for code that calls copy_{from,to}_iter > without checking the return value, introduced by commit aa28de275a24 > ("iov_iter/hardening: move object size checks to inlined part"). > > drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function > 'kiblnd_send': > drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1643:2: error: > ignoring return value of 'copy_from_iter', declared with attribute > warn_unused_result [-Werror=unused-result] > drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function > 'kiblnd_recv': > drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1744:3: error: > ignoring return value of 'copy_to_iter', declared with attribute > warn_unused_result [-Werror=unused-result] > > In case we get short copies here, we may get incorrect behavior. > I've added failure handling for both rx and tx now, returning > -EFAULT as expected. > > Cc: sta...@vger.kernel.org > Signed-off-by: Arnd Bergmann> --- > This warning now shows up in 'allmodconfig' builds, so it would be > good to get it fixed quickly for 4.13, but my patch should not go > in without careful review since I'm not familiar with with code > and the error handling is a bit tricky here. > > I added 'Cc: stable' since this is a preexisting condition that we > only started warning about now. > --- > drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 15 +-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c > b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c > index 85b242ec5f9b..70256a0ffd2e 100644 > --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c > +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c > @@ -1640,8 +1640,13 @@ kiblnd_send(struct lnet_ni *ni, void *private, struct > lnet_msg *lntmsg) > ibmsg = tx->tx_msg; > ibmsg->ibm_u.immediate.ibim_hdr = *hdr; > > - copy_from_iter(>ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE, > + rc = copy_from_iter(>ibm_u.immediate.ibim_payload, > IBLND_MSG_SIZE, > ); I have to Nak this to prevent it from landing but for some reason in my testing its returning zero from copy_from_iter. > + if (rc != IBLND_MSG_SIZE) { > + kiblnd_pool_free_node(>tx_pool->tpo_pool, >tx_list); > + return -EFAULT; > + } > + > nob = offsetof(struct kib_immediate_msg, ibim_payload[payload_nob]); > kiblnd_init_tx_msg(ni, tx, IBLND_MSG_IMMEDIATE, nob); > > @@ -1741,8 +1746,14 @@ kiblnd_recv(struct lnet_ni *ni, void *private, struct > lnet_msg *lntmsg, > break; > } > > - copy_to_iter(>ibm_u.immediate.ibim_payload, > + rc = copy_to_iter(>ibm_u.immediate.ibim_payload, >IBLND_MSG_SIZE, to); > + if (rc != IBLND_MSG_SIZE) { > + rc = -EFAULT; > + break; > + } > + > + rc = 0; > lnet_finalize(ni, lntmsg, 0); > break; > > -- > 2.9.0 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel