[PATCH] lustre: check copy_from_iter/copy_to_iter return code

[PATCH] lustre: check copy_from_iter/copy_to_iter return code

[Arnd Bergmann]

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: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
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(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
+	rc = copy_from_iter(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
 		       &from);
+	if (rc != IBLND_MSG_SIZE) {
+		kiblnd_pool_free_node(&tx->tx_pool->tpo_pool, &tx->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(&rxmsg->ibm_u.immediate.ibim_payload,
+		rc = copy_to_iter(&rxmsg->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

Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code

[James Simmons]

> 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: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> 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(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
> +	rc = copy_from_iter(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
>  		       &from);

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->tx_pool->tpo_pool, &tx->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(&rxmsg->ibm_u.immediate.ibim_payload,
> +		rc = copy_to_iter(&rxmsg->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
> 
> 

Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code

[Arnd Bergmann]

On Thu, Jul 13, 2017 at 7:07 PM, James Simmons <jsimmons@infradead.org> wrote:
>
>> 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: stable@vger.kernel.org
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> 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(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
>> +     rc = copy_from_iter(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
>>                      &from);
>
> 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

Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code

[Al Viro]

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?

Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code

[James Simmons]

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.