In C, signed integer overflow results in undefined behavior, but unsigned
overflow wraps around. So do the subtraction first, then cast to signed.
Suggested-by: Joakim Tjernlund <[email protected]>
Signed-off-by: Jim Rees <[email protected]>
---
fs/nfsd/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 316ec84..9850329 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3427,7 +3427,7 @@ grace_disallows_io(struct net *net, struct inode *inode)
/* Returns true iff a is later than b: */
static bool stateid_generation_after(stateid_t *a, stateid_t *b)
{
- return (s32)a->si_generation - (s32)b->si_generation > 0;
+ return (s32)(a->si_generation - b->si_generation) > 0;
}
static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
--
1.8.2.3
Bernd Petrovitsch wrote:
Hmm, what is actually wrong with plain-old
---- snip ----
static bool stateid_generation_after(stateid_t *a, stateid_t *b)
{
return a->si_generation > b->si_generation;
}
---- snip ----
?
It doesn't account for wraparound. Try this:
unsigned int a = 0xffff0000, b = 0x7fff0000;
printf("%d %d\n", a > b, (int)(a - b) > 0);
I just realized that I said this needed a comment, then didn't provide one.
On Fri, May 17, 2013 at 05:33:00PM -0400, Jim Rees wrote:
> In C, signed integer overflow results in undefined behavior, but unsigned
> overflow wraps around. So do the subtraction first, then cast to signed.
Thanks! Applying for 3.11.
(I wonder if it actually bites anyone in practice? Are there common
compilers or architectures where this makes a difference? Even if so I
suppose that many generations is probably unlikely enough not to make
this worth backporting to stable branches.)
--b.
>
> Suggested-by: Joakim Tjernlund <[email protected]>
> Signed-off-by: Jim Rees <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 316ec84..9850329 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3427,7 +3427,7 @@ grace_disallows_io(struct net *net, struct inode *inode)
> /* Returns true iff a is later than b: */
> static bool stateid_generation_after(stateid_t *a, stateid_t *b)
> {
> - return (s32)a->si_generation - (s32)b->si_generation > 0;
> + return (s32)(a->si_generation - b->si_generation) > 0;
> }
>
> static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
> --
> 1.8.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Bruce Fields wrote:
On Fri, May 17, 2013 at 05:33:00PM -0400, Jim Rees wrote:
> In C, signed integer overflow results in undefined behavior, but unsigned
> overflow wraps around. So do the subtraction first, then cast to signed.
Thanks! Applying for 3.11.
(I wonder if it actually bites anyone in practice? Are there common
compilers or architectures where this makes a difference? Even if so I
suppose that many generations is probably unlikely enough not to make
this worth backporting to stable branches.)
I tested on the architectures I have at my fingertips and it made no
difference. My guess is you might need a one's complement architecture. Of
course we'll never see wraparound in our lifetimes, so no sense backporting
this.
I actually used a one's complement machine many years ago, a CDC 6600. I
don't think linux has been ported to it.
On Fre, 2013-05-17 at 17:33 -0400, Jim Rees wrote:
> In C, signed integer overflow results in undefined behavior, but unsigned
> overflow wraps around. So do the subtraction first, then cast to signed.
>
> Suggested-by: Joakim Tjernlund <[email protected]>
> Signed-off-by: Jim Rees <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 316ec84..9850329 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3427,7 +3427,7 @@ grace_disallows_io(struct net *net, struct inode *inode)
> /* Returns true iff a is later than b: */
> static bool stateid_generation_after(stateid_t *a, stateid_t *b)
> {
> - return (s32)a->si_generation - (s32)b->si_generation > 0;
> + return (s32)(a->si_generation - b->si_generation) > 0;
> }
Hmm, what is actually wrong with plain-old
---- snip ----
static bool stateid_generation_after(stateid_t *a, stateid_t *b)
{
return a->si_generation > b->si_generation;
}
---- snip ----
?
Kind regards,
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at
On Sam, 2013-05-18 at 10:58 -0400, Jim Rees wrote:
> Bernd Petrovitsch wrote:
>
> Hmm, what is actually wrong with plain-old
> ---- snip ----
> static bool stateid_generation_after(stateid_t *a, stateid_t *b)
> {
> return a->si_generation > b->si_generation;
> }
> ---- snip ----
> ?
>
> It doesn't account for wraparound. Try this:
>
> unsigned int a = 0xffff0000, b = 0x7fff0000;
> printf("%d %d\n", a > b, (int)(a - b) > 0);
>
> I just realized that I said this needed a comment, then didn't provide one.
Ooops, yes, thx.
Sry for the noise;-)
Bernd
--
Bernd Petrovitsch Email : [email protected]
LUGA : http://www.luga.at