[PATCH] cifs: Fix TCP_Server_Info::credits to be signed

[PATCH] cifs: Fix TCP_Server_Info::credits to be signed

[David Howells]

Fix TCP_Server_Info::credits to be signed, just as echo_credits and
oplock_credits are.  This also fixes what ought to get at least a
compilation warning if not an outright error in *get_credits_field() as a
pointer to the unsigned server->credits field is passed back as a pointer
to a signed int.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.org>
cc: linux-cifs@vger.kernel.org
---
 fs/smb/client/cifsglob.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 8f6f567d7474..b91397dbb6aa 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -740,7 +740,7 @@ struct TCP_Server_Info {
 	bool nosharesock;
 	bool tcp_nodelay;
 	bool terminate;
-	unsigned int credits;  /* send no more requests at once */
+	int credits;  /* send no more requests at once */
 	unsigned int max_credits; /* can override large 32000 default at mnt */
 	unsigned int in_flight;  /* number of requests on the wire to server */
 	unsigned int max_in_flight; /* max number of requests that were on wire */

Re: [PATCH] cifs: Fix TCP_Server_Info::credits to be signed

[Enzo Matsumiya]

Hi David,

On 10/20, David Howells wrote:
>Fix TCP_Server_Info::credits to be signed, just as echo_credits and
>oplock_credits are.  This also fixes what ought to get at least a
>compilation warning if not an outright error in *get_credits_field() as a
>pointer to the unsigned server->credits field is passed back as a pointer
>to a signed int.

Both semantically and technically, credits shouldn't go negative.
Shouldn't those other fields/functions become unsigned instead?


Cheers,

Enzo

>Signed-off-by: David Howells <dhowells@redhat.com>
>cc: Steve French <sfrench@samba.org>
>cc: Paulo Alcantara <pc@manguebit.org>
>cc: linux-cifs@vger.kernel.org
>---
> fs/smb/client/cifsglob.h |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>index 8f6f567d7474..b91397dbb6aa 100644
>--- a/fs/smb/client/cifsglob.h
>+++ b/fs/smb/client/cifsglob.h
>@@ -740,7 +740,7 @@ struct TCP_Server_Info {
> 	bool nosharesock;
> 	bool tcp_nodelay;
> 	bool terminate;
>-	unsigned int credits;  /* send no more requests at once */
>+	int credits;  /* send no more requests at once */
> 	unsigned int max_credits; /* can override large 32000 default at mnt */
> 	unsigned int in_flight;  /* number of requests on the wire to server */
> 	unsigned int max_in_flight; /* max number of requests that were on wire */
>
>

Re: [PATCH] cifs: Fix TCP_Server_Info::credits to be signed

[David Howells]

Enzo Matsumiya <ematsumiya@suse.de> wrote:

> Both semantically and technically, credits shouldn't go negative.
> Shouldn't those other fields/functions become unsigned instead?

That's really a question for Steve, but it makes it easier to handle
underflow, and I'm guessing that the maximum credits isn't likely to exceed
2G.

David

Re: [PATCH] cifs: Fix TCP_Server_Info::credits to be signed

[Enzo Matsumiya]

On 10/20, David Howells wrote:
>Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
>> Both semantically and technically, credits shouldn't go negative.
>> Shouldn't those other fields/functions become unsigned instead?
>
>That's really a question for Steve, but it makes it easier to handle
>underflow

But if there's an overflow somewhere the math should be checked instead
(I never seen it happen though).

> and I'm guessing that the maximum credits isn't likely to exceed
>2G.

Yes, it's capped at 60-65k (depends on the function...)
So yes, an unsigned short would be fine.


Cheers,

Enzo

Re: [PATCH] cifs: Fix TCP_Server_Info::credits to be signed

[Steve French]

On Mon, Oct 20, 2025 at 9:08 AM David Howells <dhowells@redhat.com> wrote:
>
> Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> > Both semantically and technically, credits shouldn't go negative.
> > Shouldn't those other fields/functions become unsigned instead?
>
> That's really a question for Steve, but it makes it easier to handle
> underflow, and I'm guessing that the maximum credits isn't likely to exceed
> 2G.
>
> David

Interesting question - I do like the idea of keeping signed if it
makes it easier to check
for underflows but IIRC that hasn't been a problem in a long time (adding Pavel
and Ronnie in case they remember) but more important than the signed
vs. unsigned
in my opinion is at least keeping the field consistent.

I have seen a few stress related xfstests that often generate
reconnects, so we may want
to run with the tracepoint enabled
(smb3_reconnect_with_invalid_credits) to see if that
is actually happening (the underflow of credits)

I also was thinking that we should doublecheck that lease break acks
will never run out credits
(since that can deadlock servers for more than 30 seconds as they wait
for timeouts), even if
"lease break storms" are rare.   Maybe we should allow e.g. lease
break acks to borrow echo
credits e.g. as minor improvement?

-- 
Thanks,

Steve

Re: [PATCH] cifs: Fix TCP_Server_Info::credits to be signed

[Shyam Prasad N]

On Mon, Oct 20, 2025 at 10:28 PM Steve French <smfrench@gmail.com> wrote:
>
> On Mon, Oct 20, 2025 at 9:08 AM David Howells <dhowells@redhat.com> wrote:
> >
> > Enzo Matsumiya <ematsumiya@suse.de> wrote:
> >
> > > Both semantically and technically, credits shouldn't go negative.
> > > Shouldn't those other fields/functions become unsigned instead?
> >
> > That's really a question for Steve, but it makes it easier to handle
> > underflow, and I'm guessing that the maximum credits isn't likely to exceed
> > 2G.
> >
> > David
>
> Interesting question - I do like the idea of keeping signed if it
> makes it easier to check
> for underflows but IIRC that hasn't been a problem in a long time (adding Pavel
> and Ronnie in case they remember) but more important than the signed
> vs. unsigned
> in my opinion is at least keeping the field consistent.
>
> I have seen a few stress related xfstests that often generate
> reconnects, so we may want
> to run with the tracepoint enabled
> (smb3_reconnect_with_invalid_credits) to see if that
> is actually happening (the underflow of credits)
>
> I also was thinking that we should doublecheck that lease break acks
> will never run out credits
> (since that can deadlock servers for more than 30 seconds as they wait
> for timeouts), even if
> "lease break storms" are rare.   Maybe we should allow e.g. lease
> break acks to borrow echo
> credits e.g. as minor improvement?
>
> --
> Thanks,
>
> Steve

I agree with Steve.
I don't mind credits being a signed int. If credits go negative, it's
a clear indication of a bug and jumps out at you more than a large
integer would.

-- 
Regards,
Shyam