Fwd: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

Fwd: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Jamal Hadi Salim]

Sorry - I didnt mean to remove the mailing lists.
Please reply to this email instead.

cheers,
jamal

-------- Forwarded Message --------
Subject: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not 
return ENOMEM err back in sctp_packet_transmit
Date: Sun, 23 Oct 2016 11:03:36 -0400
From: Jamal Hadi Salim <jhs@mojatatu.com>
To: Xin Long <lucien.xin@gmail.com>
CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, Vlad Yasevich 
<vyasevich@gmail.com>, daniel@iogearbox.net, gabor@mojatatu.com, Brenda 
Butler <bjb@mojatatu.com>, David Miller <davem@davemloft.net>, 
linux-sctp@vger.kernel.org <linux-sctp@vger.kernel.org>, Michael Tuexen 
<tuexen@fh-muenster.de>, Eric Dumazet <edumazet@google.com>



I think the specific use case this patch addresses
seems to have bitten us in an older kernel sctp (3.11?).
A send() on a loaded network box caused the skb to
alloc in what appears to be this code path and fail (problem
is intermittent, so not 100% sure). errno seen was ENOMEM.
Unfortunately the manpage for sendxxx sucks.
It says "no memory available".
[We'll fix the manpage if there is an appropriate answer].

Two questions:
a) Seems like we can safely ignore ENOMEM in user space
at least for this use case. i.e the kernel will retry and
eventually send this message. Is there any other scenario
where we have to worry about ENOMEM showing up in user space?

b) What is the general view of what sendXXX reaction oughta
be from user space in presence of ENOMEM?

cheers,
jamal

On 16-09-08 05:44 AM, Xin Long wrote:
> As David and Marcelo's suggestion, ENOMEM err shouldn't return back to
> user in transmit path. Instead, sctp's retransmit would take care of
> the chunks that fail to send because of ENOMEM.
>
> This patch is only to do some release job when alloc_skb fails, not to
> return ENOMEM back any more.
>
> Besides, it also cleans up sctp_packet_transmit's err path, and fixes
> some issues in err path:
>
>  - It didn't free the head skb in nomem: path.
>  - No need to check nskb in no_route: path.
>  - It should goto err: path if alloc_skb fails for head.
>  - Not all the NOMEMs should free nskb.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/output.c | 47 ++++++++++++++++++++++-------------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 1934933..8f490ff 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -442,14 +442,14 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
>  			 * time. Application may notice this error.
>  			 */
>  			pr_err_once("Trying to GSO but underlying device doesn't support it.");
> -			goto nomem;
> +			goto err;
>  		}
>  	} else {
>  		pkt_size = packet->size;
>  	}
>  	head = alloc_skb(pkt_size + MAX_HEADER, gfp);
>  	if (!head)
> -		goto nomem;
> +		goto err;
>  	if (gso) {
>  		NAPI_GRO_CB(head)->last = head;
>  		skb_shinfo(head)->gso_type = sk->sk_gso_type;
> @@ -470,8 +470,12 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
>  		}
>  	}
>  	dst = dst_clone(tp->dst);
> -	if (!dst)
> -		goto no_route;
> +	if (!dst) {
> +		if (asoc)
> +			IP_INC_STATS(sock_net(asoc->base.sk),
> +				     IPSTATS_MIB_OUTNOROUTES);
> +		goto nodst;
> +	}
>  	skb_dst_set(head, dst);
>
>  	/* Build the SCTP header.  */
> @@ -622,8 +626,10 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
>  		if (!gso)
>  			break;
>
> -		if (skb_gro_receive(&head, nskb))
> +		if (skb_gro_receive(&head, nskb)) {
> +			kfree_skb(nskb);
>  			goto nomem;
> +		}
>  		nskb = NULL;
>  		if (WARN_ON_ONCE(skb_shinfo(head)->gso_segs >=
>  				 sk->sk_gso_max_segs))
> @@ -717,18 +723,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
>  	}
>  	head->ignore_df = packet->ipfragok;
>  	tp->af_specific->sctp_xmit(head, tp);
> +	goto out;
>
> -out:
> -	sctp_packet_reset(packet);
> -	return err;
> -no_route:
> -	kfree_skb(head);
> -	if (nskb != head)
> -		kfree_skb(nskb);
> -
> -	if (asoc)
> -		IP_INC_STATS(sock_net(asoc->base.sk), IPSTATS_MIB_OUTNOROUTES);
> +nomem:
> +	if (packet->auth && list_empty(&packet->auth->list))
> +		sctp_chunk_free(packet->auth);
>
> +nodst:
>  	/* FIXME: Returning the 'err' will effect all the associations
>  	 * associated with a socket, although only one of the paths of the
>  	 * association is unreachable.
> @@ -737,22 +738,18 @@ no_route:
>  	 * required.
>  	 */
>  	 /* err = -EHOSTUNREACH; */
> -err:
> -	/* Control chunks are unreliable so just drop them.  DATA chunks
> -	 * will get resent or dropped later.
> -	 */
> +	kfree_skb(head);
>
> +err:
>  	list_for_each_entry_safe(chunk, tmp, &packet->chunk_list, list) {
>  		list_del_init(&chunk->list);
>  		if (!sctp_chunk_is_data(chunk))
>  			sctp_chunk_free(chunk);
>  	}
> -	goto out;
> -nomem:
> -	if (packet->auth && list_empty(&packet->auth->list))
> -		sctp_chunk_free(packet->auth);
> -	err = -ENOMEM;
> -	goto err;
> +
> +out:
> +	sctp_packet_reset(packet);
> +	return err;
>  }
>
>  /********************************************************************
>

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Xin Long]

> I think the specific use case this patch addresses
> seems to have bitten us in an older kernel sctp (3.11?).
> A send() on a loaded network box caused the skb to
> alloc in what appears to be this code path and fail (problem
> is intermittent, so not 100% sure). errno seen was ENOMEM.
> Unfortunately the manpage for sendxxx sucks.
> It says "no memory available".
> [We'll fix the manpage if there is an appropriate answer].
>
> Two questions:
> a) Seems like we can safely ignore ENOMEM in user space
> at least for this use case. i.e the kernel will retry and
> eventually send this message. Is there any other scenario
> where we have to worry about ENOMEM showing up in user space?
>
> b) What is the general view of what sendXXX reaction oughta
> be from user space in presence of ENOMEM?
>
This patch doesn't ignore all the ENOMEN cases, only after msg is
enqueued in out queue/send queue, in the lower layer, when alloc
new skb and copy data from old skb, if it fails to alloc new skb, sctp
will ignore this ENOMEM, as this msg will be taken care by retransmit
mechanism, it's reasonable and also safe, user can't feel that.

But for the cases before enqueue, like in sctp_sendmsg,
sctp_datamsg_from_user may return ENOMEM, this err will return
back to user, and can't be ignored.

So I don't really think we should change something in manpage, what
do you think ? maybe a little explanation there is also nice, :)

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Jamal Hadi Salim]

On 16-10-23 02:20 PM, Xin Long wrote:

> This patch doesn't ignore all the ENOMEN cases, only after msg is
> enqueued in out queue/send queue, in the lower layer, when alloc
> new skb and copy data from old skb, if it fails to alloc new skb, sctp
> will ignore this ENOMEM, as this msg will be taken care by retransmit
> mechanism, it's reasonable and also safe, user can't feel that.
>

Yes, that part i got.

> But for the cases before enqueue, like in sctp_sendmsg,
> sctp_datamsg_from_user may return ENOMEM, this err will return
> back to user, and can't be ignored.
>

The hard part is distinguishing between the above case and real
failure.
I am assuming in the case above user is _not_ required to send
again. But in the general case they are required to send again.
Correct?

> So I don't really think we should change something in manpage, what
> do you think ? maybe a little explanation there is also nice, :)

Yes, that would help. In particular it should be clear what user space
is expected to do. While this is about sctp - I am assuming equivalent
behavior for all callers of sendxxx() regardless of protocol.

cheers,
jamal

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Xin Long]

[1]
>> This patch doesn't ignore all the ENOMEN cases, only after msg is
>> enqueued in out queue/send queue, in the lower layer, when alloc
>> new skb and copy data from old skb, if it fails to alloc new skb, sctp
>> will ignore this ENOMEM, as this msg will be taken care by retransmit
>> mechanism, it's reasonable and also safe, user can't feel that.
>>
>
> Yes, that part i got.
>

[2]
>> But for the cases before enqueue, like in sctp_sendmsg,
>> sctp_datamsg_from_user may return ENOMEM, this err will return
>> back to user, and can't be ignored.
>>
>
> The hard part is distinguishing between the above case and real
> failure.
> I am assuming in the case above user is _not_ required to send
> again. But in the general case they are required to send again.
> Correct?
in case [1], user can't see the ENOMEM, ENOMEM is more like
a internal err.

in case [2], user will got the ENOMEM, they should resend this msg,
It's the the general case mentioned-above

>
>> So I don't really think we should change something in manpage, what
>> do you think ? maybe a little explanation there is also nice, :)
>
>
> Yes, that would help. In particular it should be clear what user space
> is expected to do. While this is about sctp - I am assuming equivalent
> behavior for all callers of sendxxx() regardless of protocol.
here sctp's behavior is actually same with tcp's, in tcp, tcp_transmit_skb
also may fail to alloc skb, but it doesn't return any err to user, just like
sctp_packet_transmit. That's why I don't think we should change something
in manpage, as here sctp is consistent with tcp now.

make sense ?

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Marcelo Ricardo Leitner]

On Mon, Oct 24, 2016 at 02:30:07PM +0800, Xin Long wrote:
> [1]
> >> This patch doesn't ignore all the ENOMEN cases, only after msg is
> >> enqueued in out queue/send queue, in the lower layer, when alloc
> >> new skb and copy data from old skb, if it fails to alloc new skb, sctp
> >> will ignore this ENOMEM, as this msg will be taken care by retransmit
> >> mechanism, it's reasonable and also safe, user can't feel that.
> >>
> >
> > Yes, that part i got.
> >
> 
> [2]
> >> But for the cases before enqueue, like in sctp_sendmsg,
> >> sctp_datamsg_from_user may return ENOMEM, this err will return
> >> back to user, and can't be ignored.
> >>
> >
> > The hard part is distinguishing between the above case and real
> > failure.
> > I am assuming in the case above user is _not_ required to send
> > again. But in the general case they are required to send again.
> > Correct?
> in case [1], user can't see the ENOMEM, ENOMEM is more like
> a internal err.
> 
> in case [2], user will got the ENOMEM, they should resend this msg,
> It's the the general case mentioned-above
> 
> >
> >> So I don't really think we should change something in manpage, what
> >> do you think ? maybe a little explanation there is also nice, :)
> >
> >
> > Yes, that would help. In particular it should be clear what user space
> > is expected to do. While this is about sctp - I am assuming equivalent
> > behavior for all callers of sendxxx() regardless of protocol.
> here sctp's behavior is actually same with tcp's, in tcp, tcp_transmit_skb
> also may fail to alloc skb, but it doesn't return any err to user, just like
> sctp_packet_transmit. That's why I don't think we should change something
> in manpage, as here sctp is consistent with tcp now.
> 
> make sense ?

I may be saying what is already understood, but just to be clear,
without this patch, there is no consistent way to known if you hit [1]
or [2]. Recovering from it then depends on how the protocol above SCTP
will handle it, if it can handle duplicate messages or not.

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Jamal Hadi Salim]

On 16-10-24 02:30 AM, Xin Long wrote:

> in case [1], user can't see the ENOMEM, ENOMEM is more like
> a internal err.
>

Still not clear. Are you saying, say an old kernel like 3.11 would
not return the user ENOMEN for the use case[1] you fixed? I am not
talking post your fix.

> in case [2], user will got the ENOMEM, they should resend this msg,
> It's the the general case mentioned-above
>

I am trying to see if we can avoid backporting this fix to 3.11.
In [1], is ENOMEM propagated to user space (dont talk about your
fix, I mean pre-your-fix).


> here sctp's behavior is actually same with tcp's, in tcp, tcp_transmit_skb
> also may fail to alloc skb, but it doesn't return any err to user, just like
> sctp_packet_transmit. That's why I don't think we should change something
> in manpage, as here sctp is consistent with tcp now.
>
> make sense ?

No ;-> The manpage is bad. Go look at it. In the case of ENOBUFS or
EMSGSIZE it is clear what needs to be done.
If the answer is _on ENOMEM_ user must resend then thats what we need
to say.

cheers,
jamal

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Xin Long]

>> in case [1], user can't see the ENOMEM, ENOMEM is more like
>> a internal err.
>>
>
> Still not clear. Are you saying, say an old kernel like 3.11 would
> not return the user ENOMEN for the use case[1] you fixed? I am not
> talking post your fix.
Sorry for confusing you.

3.11 would return the user ENOMEN for the use case[1].
but this behavior is incorrect, it's not consistent with tcp.

>
>> in case [2], user will got the ENOMEM, they should resend this msg,
>> It's the the general case mentioned-above
>>
>
> I am trying to see if we can avoid backporting this fix to 3.11.
> In [1], is ENOMEM propagated to user space (dont talk about your
> fix, I mean pre-your-fix).
yes, in [1], pre-my-fix, ENOMEM is propagated to user space.

>
>
>> here sctp's behavior is actually same with tcp's, in tcp, tcp_transmit_skb
>> also may fail to alloc skb, but it doesn't return any err to user, just
>> like
>> sctp_packet_transmit. That's why I don't think we should change something
>> in manpage, as here sctp is consistent with tcp now.
>>
>> make sense ?
>
>
> No ;-> The manpage is bad. Go look at it. In the case of ENOBUFS or
> EMSGSIZE it is clear what needs to be done.
> If the answer is _on ENOMEM_ user must resend then thats what we need
> to say.
yes, on ENOMEM user must resend if he want send out this msg successfully.

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Marcelo Ricardo Leitner]

On Tue, Oct 25, 2016 at 05:05:41PM +0800, Xin Long wrote:
> >> in case [1], user can't see the ENOMEM, ENOMEM is more like
> >> a internal err.
> >>
> >
> > Still not clear. Are you saying, say an old kernel like 3.11 would
> > not return the user ENOMEN for the use case[1] you fixed? I am not
> > talking post your fix.
> Sorry for confusing you.
> 
> 3.11 would return the user ENOMEN for the use case[1].
> but this behavior is incorrect, it's not consistent with tcp.
> 
> >
> >> in case [2], user will got the ENOMEM, they should resend this msg,
> >> It's the the general case mentioned-above
> >>
> >
> > I am trying to see if we can avoid backporting this fix to 3.11.
> > In [1], is ENOMEM propagated to user space (dont talk about your
> > fix, I mean pre-your-fix).
> yes, in [1], pre-my-fix, ENOMEM is propagated to user space.
> 
> >
> >
> >> here sctp's behavior is actually same with tcp's, in tcp, tcp_transmit_skb
> >> also may fail to alloc skb, but it doesn't return any err to user, just
> >> like
> >> sctp_packet_transmit. That's why I don't think we should change something
> >> in manpage, as here sctp is consistent with tcp now.
> >>
> >> make sense ?
> >
> >
> > No ;-> The manpage is bad. Go look at it. In the case of ENOBUFS or
> > EMSGSIZE it is clear what needs to be done.
> > If the answer is _on ENOMEM_ user must resend then thats what we need
> > to say.
> yes, on ENOMEM user must resend if he want send out this msg successfully.

Thing is, it may lead to duplicate messages in Application layer, as the
msg that was errored out may have been actually queued and later
retransmitted.

That's why I said the recovery steps from this depends on the
application on top of SCTP, if it can handle such duplicate messages or
not.

  Marcelo

Re: send/sendmsg ENOMEM errors WAS(Re: [PATCH net 6/6] sctp: not return ENOMEM err back in sctp_packet_transmit

[Jamal Hadi Salim]

On 16-10-25 06:34 AM, Marcelo Ricardo Leitner wrote:
> On Tue, Oct 25, 2016 at 05:05:41PM +0800, Xin Long wrote:
>>>> in case [1], user can't see the ENOMEM, ENOMEM is more like

>
> Thing is, it may lead to duplicate messages in Application layer, as the
> msg that was errored out may have been actually queued and later
> retransmitted.
>
> That's why I said the recovery steps from this depends on the
> application on top of SCTP, if it can handle such duplicate messages or
> not.

Yes, I was worried about duplicate messages.
Which is a bug on SCTP implementation on Linux, unfortunately. IOW,
transport should take care of duplicates - not the app.
i.e any app change is a workaround which will be  unnecessary in newer
kernels.

cheers,
jamal