Re: [tipc-discussion] [PATCH RFC 2/2] tipc: fix changeover issues due to large packet

[Ying X. <yin...@wi...>]

> 2) The same scenario above can happen more easily in case the MTU of
> the links is set differently or when changing. In that case, as long as
> a large message in the failure link's transmq queue was built and
> fragmented with its link's MTU > the other link's one, the issue will
> happen (there is no need of a link synching in advance).
> 
> 3) The link synching procedure also faces with the same issue but since
> the link synching is only started upon receipt of a SYNCH_MSG, dropping
> the message will not result in a state deadlock, but it is not expected
> as design.
> 
> The 1) & 3) issues are resolved by the previous commit 81e4dd94b214

This is the same as previous commit. The commit ID might be invalid
after it's merged into upstream.

> ("tipc: optimize link synching mechanism") by generating only a dummy
> SYNCH_MSG (i.e. without data) at the link synching, so the size of a
> FAILOVER_MSG if any then will never exceed the link's MTU.

>  /**
> + * tipc_msg_fragment - build a fragment skb list for TIPC message
> + *
> + * @skb: TIPC message skb
> + * @hdr: internal msg header to be put on the top of the fragments
> + * @pktmax: max size of a fragment incl. the header
> + * @frags: returned fragment skb list
> + *
> + * Returns 0 if the fragmentation is successful, otherwise: -EINVAL
> + * or -ENOMEM
> + */
> +int tipc_msg_fragment(struct sk_buff *skb, const struct tipc_msg *hdr,
> +		      int pktmax, struct sk_buff_head *frags)
> +{
> +	int pktno, nof_fragms, dsz, dmax, eat;
> +	struct tipc_msg *_hdr;
> +	struct sk_buff *_skb;
> +	u8 *data;
> +
> +	/* Non-linear buffer? */
> +	if (skb_linearize(skb))
> +		return -ENOMEM;
> +
> +	data = (u8 *)skb->data;
> +	dsz = msg_size(buf_msg(skb));
> +	dmax = pktmax - INT_H_SIZE;
> +
> +	if (dsz <= dmax || !dmax)
> +		return -EINVAL;
> +
> +	nof_fragms = dsz / dmax + 1;
> +
> +	for (pktno = 1; pktno <= nof_fragms; pktno++) {
> +		if (pktno < nof_fragms)
> +			eat = dmax;
> +		else
> +			eat = dsz % dmax;
> +
> +		_skb = tipc_buf_acquire(INT_H_SIZE + eat, GFP_ATOMIC);
> +		if (!_skb)
> +			goto error;
> +
> +		skb_orphan(_skb);
> +		__skb_queue_tail(frags, _skb);
> +
> +		skb_copy_to_linear_data(_skb, hdr, INT_H_SIZE);
> +		skb_copy_to_linear_data_offset(_skb, INT_H_SIZE, data, eat);
> +		data += eat;
> +
> +		_hdr = buf_msg(_skb);
> +		msg_set_fragm_no(_hdr, pktno);
> +		msg_set_nof_fragms(_hdr, nof_fragms);
> +		msg_set_size(_hdr, INT_H_SIZE + eat);
> +	}
> +	return 0;
> +

In fact we have similar code in tipc_msg_build() where we also fragment
packet if necessary. In order to eliminate redundant code, I suggest we
should extract the common code into a separate function and then
tipc_msg_build() and tipc_msg_fragment() call it.