[Open-FCoE] [PATCH] libfc: Validating SOF, EOF, SEQ_ID, SEQ_CNT, f_ctl for response frames

Joe Eykholt jeykholt at cisco.com
Wed Jul 15 21:40:22 UTC 2009


Steve Ma wrote:
> This patch is for handling the received frames where the other end
> is originating the sequence in response to the exchange originated
> by the initiator.
> 
> The current code is weak in validation of SOF,EOF,SEQ_ID,SEQ_CNT,f_ctl
> for response frames, or no validations for SEQ_CNT.
> 
> This patch is to add code to perform the validations of f_ctl in the
> frame header for FC_FC_LAST_SEQ, FC_FC_END_SEQ, and FC_FC_SEQ_INIT
> bits. The frame will be dropped (i.e. freed) if it fails the validation.
> 
> Signed-off-by: Steve Ma <steve.ma at intel.com>
> ---
> 
>  drivers/scsi/libfc/fc_exch.c |   59 ++++++++++++++++++++++++++++++++++++------
>  1 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
> index f42695e..d86eda8 100644
> --- a/drivers/scsi/libfc/fc_exch.c
> +++ b/drivers/scsi/libfc/fc_exch.c
> @@ -1124,7 +1124,9 @@ static void fc_exch_recv_seq_resp(struct fc_exch_mgr *mp, struct fc_frame *fp)
>  	struct fc_seq *sp;
>  	struct fc_exch *ep;
>  	enum fc_sof sof;
> +	enum fc_eof eof;
>  	u32 f_ctl;
> +	u16 cnt;
>  	void (*resp)(struct fc_seq *, struct fc_frame *fp, void *arg);
>  	void *ex_resp_arg;
>  	int rc;
> @@ -1149,29 +1151,70 @@ static void fc_exch_recv_seq_resp(struct fc_exch_mgr *mp, struct fc_frame *fp)
>  		atomic_inc(&mp->stats.xid_not_found);
>  		goto rel;
>  	}
> +
>  	sof = fr_sof(fp);
> -	if (fc_sof_is_init(sof)) {
> +	eof = fr_eof(fp);
> +	cnt = ntohs(fh->fh_seq_cnt);
> +	f_ctl = ntoh24(fh->fh_f_ctl);
> +
> +	if ((sof ==  FC_SOF_I3 && eof == FC_EOF_T)) {

Remove extra parens and extra space after the first ==.

	if (sof == FC_SOF_I3 && eof == FC_EOF_T) {

> +		/* first and last frame in a sequence */
>  		sp = fc_seq_start_next(&ep->seq);
> +		if ((cnt && (cnt != sp->cnt + 1)) ||
> +		    (!(f_ctl & FC_FC_END_SEQ))) {

Since it's the only frame of the sequence, shouldn't cnt be zero?

		if (!cnt || !(f_ctl & FC_FC_END_SEQ)) {

Avoid extra parens.

> +			atomic_inc(&mp->stats.seq_not_found);
> +			goto rel;
> +		}
> +		sp->cnt = cnt;
>  		sp->id = fh->fh_seq_id;
>  		sp->ssb_stat |= SSB_ST_RESP;
> -	} else {
> +		ep->esb_stat |= ESB_ST_SEQ_INIT;
> +	} else if ((sof ==  FC_SOF_I3 && eof == FC_EOF_N)) {

remove extra parens and extra space after first ==

> +		/* first and not last frame in a sequence */
> +		sp = fc_seq_start_next(&ep->seq);
> +		if ((cnt && (cnt != sp->cnt + 1)) ||

Again, shouldn't cnt be zero?

> +		    (f_ctl & (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) ||

LAST_SEQ is allowed on any frame in the last sequence, although
it is only required on the last frame.  So, don't check that here.
See FC-FS-2, section 9.7.5.

> +		    (f_ctl & FC_FC_SEQ_INIT)) {
> +			atomic_inc(&mp->stats.seq_not_found);
> +			goto rel;
> +		}
> +		sp->cnt = cnt;
> +		sp->id = fh->fh_seq_id;
> +		sp->ssb_stat |= SSB_ST_RESP;
> +		ep->esb_stat |= ESB_ST_SEQ_INIT;
> +	} else if ((sof ==  FC_SOF_N3) && (eof == FC_EOF_N)) {
> +		/* middle frame in a sequence */
>  		sp = &ep->seq;
> -		if (sp->id != fh->fh_seq_id) {
> +		if ((sp->id != fh->fh_seq_id) ||
> +		    (sp->cnt + 1 != cnt) ||
> +		    (f_ctl & (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) ||
> +		    (f_ctl & FC_FC_SEQ_INIT)) {

Don't check LAST_SEQ here, either.
Suggest removing extra parens around comparisons.

> +			atomic_inc(&mp->stats.seq_not_found);
> +			goto rel;
> +		}
> +		sp->cnt++;
> +	} else if ((sof ==  FC_SOF_N3) && (eof == FC_EOF_T)) {
> +		/* last frame in a sequence */
> +		sp = &ep->seq;
> +		if ((sp->id != fh->fh_seq_id) ||
> +		    (sp->cnt + 1 != cnt) ||

We do see reordering of frames for FCP when interrupt
migration moves the receive interrupt to another CPU.
So the sequence comparison shouldn't be done unless we somehow
figure out how to preserve order in this case.

BTW, the only non-FCP sequences that we handle as multiple frames
are CT GPN_FT responses.  fc_disc.c already checks that they
arrive in order, so we really don't need to check it here.

> +		    (!(f_ctl & FC_FC_END_SEQ))) {
>  			atomic_inc(&mp->stats.seq_not_found);
>  			goto rel;
>  		}
> +		sp->cnt++;

Need to test for SEQ_INIT here, too.

> +	} else {
> +		atomic_inc(&mp->stats.seq_not_found);
> +		goto rel;
>  	}
> -	f_ctl = ntoh24(fh->fh_f_ctl);
> -	fr_seq(fp) = sp;
> -	if (f_ctl & FC_FC_SEQ_INIT)
> -		ep->esb_stat |= ESB_ST_SEQ_INIT;
>  
> +	fr_seq(fp) = sp;
>  	if (fc_sof_needs_ack(sof))
>  		fc_seq_send_ack(sp, fp);
>  	resp = ep->resp;
>  	ex_resp_arg = ep->arg;
>  
> -	if (fh->fh_type != FC_TYPE_FCP && fr_eof(fp) == FC_EOF_T &&
> +	if (fh->fh_type != FC_TYPE_FCP && eof == FC_EOF_T &&
>  	    (f_ctl & (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) ==
>  	    (FC_FC_LAST_SEQ | FC_FC_END_SEQ)) {
>  		spin_lock_bh(&ep->ex_lock);
> 
> _______________________________________________
> devel mailing list
> devel at open-fcoe.org
> http://www.open-fcoe.org/mailman/listinfo/devel




More information about the devel mailing list