Ma, Steve wrote:
>
>> -----Original Message-----
>> From: Joe Eykholt [mailto:[email protected]]
>> Sent: Wednesday, July 15, 2009 2:40 PM
>> To: Ma, Steve
>> Cc: [email protected]
>> Subject: Re: [Open-FCoE] [PATCH] libfc: Validating SOF, EOF, SEQ_ID,
>> SEQ_CNT, f_ctl for response frames
>>
>> 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 <[email protected]>
>>> ---
>>>
>>> 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?
>
> Because the sequence may not be the first sequence of the exchange,
> the SEQ_CNT can be either zero or continuously increasing. FC-FS-2, 9.10.
Good point. I forgot about continuously increasing sequence count.
>> 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?
>>
> Same as above.
>
>>> + (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.
>>
> I have also seen multiple response frames when sending ECHO of 256 bytes to a
> SANBlaze target.
OK, sure. Still, we can get reordering, so checking sequence count is a
problem.
I think it's better to let the upper level (FCP or fc_disc or fcping) handle it.
>>> + (!(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
>>> [email protected]
>>> http://www.open-fcoe.org/mailman/listinfo/devel
>
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
