On Sat, Jan 23, 2021 at 5:47 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>
> Ronnie,
>
> I still think that your patch needs additional fix here:
>
> -----------
> /*
> * If signal is pending but we have already sent the whole packet to
> * the server we need to return success status to allow a corresponding
> * mid entry to be kept in the pending requests queue thus allowing
> * to handle responses from the server by the client.
> *
> * If only part of the packet has been sent there is no need to hide
> * interrupt because the session will be reconnected anyway, so there
> * won't be any response from the server to handle.
> */
>
> if (signal_pending(current) && (total_len != send_length)) {
> cifs_dbg(FYI, "signal is pending after attempt to send\n");
> rc = -EINTR;
> ^^^
> This should be changed to -ERESTARTSYS to allow kernel to
> restart a syscall.
>
> }
> -----------
>
> Since the signal may remain pending when we enter the sending loop, we
> may end up not sending the whole packet before TCP buffers become
> full. In this case according to the condition above the code returns
> -EINTR but what we need here is to return -ERESTARTSYS instead to
> allow system calls to be restarted.
>
> Thoughts?
Yes, that is probably a good idea to change too.
Steve, can you add this change to my patch?
>
> --
> Best regards,
> Pavel Shilovsky
>
> чт, 21 янв. 2021 г. в 14:41, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
> >
> >
> >
> > On Thu, Jan 21, 2021 at 10:19 PM Paulo Alcantara <pc@xxxxxx> wrote:
> >>
> >> Aurélien Aptel <aaptel@xxxxxxxx> writes:
> >>
> >> > Pavel Shilovsky <piastryyy@xxxxxxxxx> writes:
> >> >
> >> >> вт, 19 янв. 2021 г. в 22:38, Steve French <smfrench@xxxxxxxxx>:
> >> >>>
> >> >>> The patch won't merge (also has some text corruptions in it). This
> >> >>> line of code is different due to commit 6988a619f5b79
> >> >>>
> >> >>> 6988a619f5b79 (Paulo Alcantara 2020-11-28 15:57:06 -0300 342)
> >> >>> cifs_dbg(FYI, "signal pending before send request\n");
> >> >>> 6988a619f5b79 (Paulo Alcantara 2020-11-28 15:57:06 -0300 343)
> >> >>> return -ERESTARTSYS;
> >> >>>
> >> >>> if (signal_pending(current)) {
> >> >>> cifs_dbg(FYI, "signal pending before send request\n");
> >> >>> return -ERESTARTSYS;
> >> >>> }
> >> >>>
> >> >>> See:
> >> >>>
> >> >>> Author: Paulo Alcantara <pc@xxxxxx>
> >> >>> Date: Sat Nov 28 15:57:06 2020 -0300
> >> >>>
> >> >>> cifs: allow syscalls to be restarted in __smb_send_rqst()
> >> >>>
> >> >>> A customer has reported that several files in their multi-threaded app
> >> >>> were left with size of 0 because most of the read(2) calls returned
> >> >>> -EINTR and they assumed no bytes were read. Obviously, they could
> >> >>> have fixed it by simply retrying on -EINTR.
> >> >>>
> >> >>> We noticed that most of the -EINTR on read(2) were due to real-time
> >> >>> signals sent by glibc to process wide credential changes (SIGRT_1),
> >> >>> and its signal handler had been established with SA_RESTART, in which
> >> >>> case those calls could have been automatically restarted by the
> >> >>> kernel.
> >> >>>
> >> >>> Let the kernel decide to whether or not restart the syscalls when
> >> >>> there is a signal pending in __smb_send_rqst() by returning
> >> >>> -ERESTARTSYS. If it can't, it will return -EINTR anyway.
> >> >>>
> >> >>> Signed-off-by: Paulo Alcantara (SUSE) <pc@xxxxxx>
> >> >>> CC: Stable <stable@xxxxxxxxxxxxxxx>
> >> >>> Reviewed-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> >> >>> Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
> >> >>>
> >> >>> On Tue, Jan 19, 2021 at 10:32 PM Ronnie Sahlberg <lsahlber@xxxxxxxxxx> wrote:
> >> >>> >
> >> >>> > RHBZ 1848178
> >> >>> >
> >> >>> > There is no need to fail this function if non-fatal signals are
> >> >>> > pending when we enter it.
> >> >>> >
> >> >>> > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> >> >>> > ---
> >> >>> > fs/cifs/transport.c | 2 +-
> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>> >
> >> >>> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> >>> > index c42bda5a5008..98752f7d2cd2 100644
> >> >>> > --- a/fs/cifs/transport.c
> >> >>> > +++ b/fs/cifs/transport.c
> >> >>> > @@ -339,7 +339,7 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
> >> >>> > if (ssocket == NULL)
> >> >>> > return -EAGAIN;
> >> >>> >
> >> >>> > - if (signal_pending(current)) {
> >> >>> > + if (fatal_signal_pending(current)) {
> >> >>> > cifs_dbg(FYI, "signal is pending before sending any data\n");
> >> >>> > return -EINTR;
> >> >>> > }
> >> >
> >> > I've looked up the difference
> >> >
> >> > static inline int __fatal_signal_pending(struct task_struct *p)
> >> > {
> >> > return unlikely(sigismember(&p->pending.signal, SIGKILL));
> >> > }
> >> >
> >> >
> >> >> I have been thinking around the same lines. The original intent of
> >> >> failing the function here was to avoid interrupting packet send in the
> >> >> middle of the packet and not breaking an SMB connection.
> >> >> That's also why signals are blocked around smb_send_kvec() calls. I
> >> >> guess most of the time a socket buffer is not full, so, those
> >> >> functions immediately return success without waiting internally and
> >> >> checking for pending signals. With this change the code may break SMB
> >> >
> >> > Ah, interesting.
> >> >
> >> > I looked up the difference between fatal/non-fatal and it seems
> >> > fatal_signal_pending() really only checks for SIGKILL, but I would
> >> > expect ^C (SIGINT) to return quickly as well.
> >> >
> >> > I thought the point of checking for pending signal early was to return
> >> > quickly to userspace and not be stuck in some unkillable state.
> >> >
> >> > After reading your explanation, you're saying the kernel funcs to send
> >> > on socket will check for any signal and err early in any case.
> >> >
> >> > some_syscall() {
> >> >
> >> > if (pending_fatal_signal) <===== if we ignore non-fatal here
> >> > fail_early();
> >> >
> >> > block_signals();
> >> > r = kernel_socket_send {
> >> > if (pending_signal) <==== they will be caught here
> >> > return error;
> >> >
> >> > ...
> >> > }
> >> > unblock_signals();
> >> > if (r)
> >> > fail();
> >> > ...
> >> > }
> >> >
> >> > So this patch will (potentially) trigger more reconnect (because we
> >> > actually send the packet as a vector in a loop) but I'm not sure I
> >> > understand why it returns less errors to userspace?
> >> >
> >> > Also, shouldn't we move the pending_fatal_signal check *inside* the blocked
> >> > signal section?
> >> >
> >> > In any case I think we should try to test some of those changes given
> >> > how we have 3,4 patches trying to tweak it on top of each other.
> >>
> >> I think it would make sense to have something like
> >>
> >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> index e9abb41aa89b..f7292c14863e 100644
> >> --- a/fs/cifs/transport.c
> >> +++ b/fs/cifs/transport.c
> >> @@ -340,7 +340,7 @@ __smb_send_rqst(struct TCP_Server_Info *server, int num_rqst,
> >>
> >> if (signal_pending(current)) {
> >> cifs_dbg(FYI, "signal pending before send request\n");
> >> - return -ERESTARTSYS;
> >> + return __fatal_signal_pending(current) ? -EINTR : -ERESTARTSYS;
> >> }
> >>
> >
> > That is not sufficient because there are syscalls that are never supposed to fail with -EINTR or -ERESTARTSYS
> > and thus will not be restarted by either the kernel or libc.
> >
> > For example utimensat(). The change to only fail here with -E* for a fatal signal (when the process will be killed anyway)
> > is to address an issue when IF there are signals pending, any signal, during the utimensat() system call then
> > this will lead to us returning -EINTR back to the application. Which can break some applications such as 'tar'.
> >
> >
> > ronnie s
> >
> >
> >>
> >> /* cork the socket */
> >>
> >> so that we allow signal handlers to be executed before restarting
> >> syscalls when receiving non-fatal signals, otherwise -EINTR.
